[issue-184] validate against json-spec #254
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
armintaenzertng
previously requested changes
Oct 27, 2022
spdx/file.py
Outdated
| @@ -63,7 +63,7 @@ def __init__(self, name, spdx_id=None, chk_sum=None): | |||
| self.spdx_id = spdx_id | |||
| self.comment = None | |||
| self.type = None | |||
| self.chk_sum = chk_sum | |||
| self.checksums = [None] | |||
There was a problem hiding this comment.
Suggested change
| self.checksums = [None] | |
| self.checksums = [chk_sum] |
and maybe rename chk_sum to checksum
There was a problem hiding this comment.
I think the intention here was to distinguish the package and file checksum by using different names chk_sum for file and check_sum for package. I agree that it is better to use chksum and checksum but I would stick to the different names for the objects since they are slightly different: checksums for packages are optional whereas checksums for files are required.
There was a problem hiding this comment.
To give my opinion here (already discussed with @meretp ):
- I find the distinction between the two variable names rather arbitrary and obscure. I would question whether it's necessary at all, and if it is, I would prefer something like
file_checksumandpackage_checksum. That would make it clear to a reader which one is which, and also provide a hint at why the naming distinction is made in the first place - Considering that this is probably an issue in quite a lot of places, and we are planning a major redesign in the near future, I want to be pragmatic about this. If renaming the variables everywhere is a lot of effort, let's just stick with the status quo until the redesign.
Closed
nicoweidner
dismissed
armintaenzertng’s stale review
Currently sick and unable to do another round
nicoweidner
previously approved these changes
Nov 2, 2022
There was a problem hiding this comment.
I just checked the outstanding comments, since @armintaenzertng already did an extensive review. I think this is good to go now (as soon as appveyor starts working again or we disable it somehow...)
Closed
nicoweidner
previously approved these changes
Nov 3, 2022
Signed-off-by: Daniel Holth <[email protected]>
Signed-off-by: Meret Behrens <[email protected]>
leads to failing tests caused by jsonyamlxml writer since json and yaml expect surrounding document Signed-off-by: Meret Behrens <[email protected]>
This leads to the failure of tests caused by the jsonyamlxml writer. Signed-off-by: Meret Behrens <[email protected]>
…spec 2.2 Signed-off-by: Meret Behrens <[email protected]>
…ing to spec Signed-off-by: Meret Behrens <[email protected]>
Signed-off-by: Meret Behrens <[email protected]>
…lidate against spec Signed-off-by: Meret Behrens <[email protected]>
Signed-off-by: Meret Behrens <[email protected]>
- correct checksum in json example - add JSONExample2.2 from spec but exclude in tests since 2.2 is not yet completely supported - add files only once if they appear in multiple packages - parse only spdxid in documentDescribes, delete commented out code - delete unused XMLWriter and JsonYamlWriter class, updated xml test results - rework create_document_describes method - delete surrounding document in json/yaml test - rename licenseinfoinfiles method according to variable - rename chk_sum/ check_sum to chksum/checksum - delete duplicated relationships from json/yaml/xml Signed-off-by: Meret Behrens <[email protected]>
This was referenced Nov 7, 2022
Merged
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR is taken over from the original PR #197 by @dholth.
Besides writing the json according to the spec this PR also handles parsing json/yaml/xml files.
This PR doesn't cover files that do not belong to any package since this would need more work on the internal data model. Also it is not possible to parse checksums other than sha1, there is another PR #200 dealing with this, which can hopefully be rebased on this and add this functionality.
fixes #184, #234,#240, #242.