Skip to content

Refactoring based on versions #165

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
kzantow opened this issue Oct 10, 2022 · 4 comments · Fixed by #172
Closed

Refactoring based on versions #165

kzantow opened this issue Oct 10, 2022 · 4 comments · Fixed by #172

Comments

@kzantow
Copy link
Collaborator

kzantow commented Oct 10, 2022

I've implemented 2.3 support with this PR. This was not a particularly easy task, as there are a number of inconsistencies in the project layout. It is also not easy to see the differences between the 2.2 and 2.3 code paths. I have a suggestion to refactor this repository such that the layout uses versions at the top level, e.g. something like:

common
v2/v2_1
v2/v2_2
v2/v2_3
v2/v2_3/idsearcher/...
v2/v2_3/json/reader.go (with a `Read` function)
v2/v2_3/json/writer.go (with a `Write` function)
v2/v2_3/rdf/reader.go (with a `Read` function)

Where each version contains all the version specific files. This avoids files growing uncontrollably like the idsearcher as every version increases (there are a number of files like this). It's also unclear when implementing a new version exactly which files need to be modified vs. copied.

There are also inconsistencies like the json package containing a writer and parser but separate packages for tvloader and tvsaver, there should be a common pattern here (I think the json package having a reader and writer is the better pattern). If things are split up by version, then this becomes even easier, as you just reference <version>/<format> and have access to a read and write functions and they don't even have to be renamed (just Read/Parse and Write), so these function names with 2_2 and 2_3 aren't things that show up in the diffs between the versions.

If all the code specific to a version was within a specific path, adding a new version would be as simple as copying the previous one and making appropriate changes. It would then be easy to see the differences between them using something like: diff v2/v2_2 v2/v2_3 --ignore-matching-lines='package v2_.'

This means for any consumer, all they would need to do is update the paths, like from something like v2_2 to v2_3, the other function calls would remain the same.

I would be happy to take on this refactoring if it is something that might be accepted.

@kzantow
Copy link
Collaborator Author

kzantow commented Oct 10, 2022

NOTE: this is a similar but somewhat different version of #152

@kzantow
Copy link
Collaborator Author

kzantow commented Oct 10, 2022

I've made a draft PR to illustrate these changes here: kzantow-anchore#1

I would not be opposed to removing the v2 subdirectory, so things like 2.3 are v2_3 instead of v2/v2_3, if that is something desired.

This PR is also incomplete:

  • many of the comments have erroneous things that should be removed to make transition to a new spec easier
  • many of the files with multiple spec versions like idsearcher BuildIDsDocument2_1, BuildIDsDocument2_2, etc. have been left in place but these would be moved so each version has its own entry point in a version subdirectory for these functions in order to have a PR that is intended to be merged

CC: @lumjjb @swinslow

@kzantow
Copy link
Collaborator Author

kzantow commented Oct 10, 2022

One more note, if looking at this code change:
image

The following top-level directories would be moved underneath the appropriate version, but I haven't done this yet:

  • builder
  • idsearcher
  • licensediff
  • reporter
  • spdxlib
  • utils (verification)

The following top-level directories are empty due to files being moved:

  • json
  • rdfloader
  • tvloader
  • tvsaver
  • yaml

So the final top-level structure is only 4 directories:

  • common
  • examples (which might be better moved per-version, also)
  • utils (which could probably be moved under common)
  • v2

@lumjjb
Copy link
Collaborator

lumjjb commented Oct 11, 2022

Hi @kzantow - thanks for diving deep into this!! Appreciate all the work and I hope to get in some comments on the 2.3 PR by this Wednesday! Thanks for taking the time to do some analysis and think through a refactor - I do agree that it needs some work!

I would not be opposed to removing the v2 subdirectory, so things like 2.3 are v2_3 instead of v2/v2_3, if that is something desired.

We are expecting v3 to be out in the next year or so, which would be incompatible with v2, which was the reason we created a separate folder for it.

The following top-level directories would be moved underneath the appropriate version, but I haven't done this yet:

We do have thoughts on generalizing these functions (see #152), and when generics based on struct fields are a thing, we would move to that from #152. The hope is that the "core" model can deduplicate all these code so they can generically work for any SPDX major version. I would like to hear your thoughts on that and see if you have any ideas to incorporate. I was writing some tricky reflection code and kind of gave up half way... 😂

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants