-
Notifications
You must be signed in to change notification settings - Fork 144
Make 2.3 optional properties optional #276
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me! Only three minor comments, although I am open to discuss whether the parser functions for the mandatory fields should be called (to match the previous style) or not.
spdx/package.py
Outdated
else: | ||
for f in self.files: | ||
messages = f.validate(messages) | ||
for f in self.files: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably use file
here since we iterate over files.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
@@ -658,7 +658,7 @@ def parse_snippet_license_info_from_snippet(self, license_info_from_snippet): | |||
self.value_error("SNIPPET_LIC_INFO", lic_in_snippet) | |||
else: | |||
self.value_error("SNIPPET_LIC_INFO", lic_in_snippet) | |||
else: | |||
elif license_info_from_snippet is not None: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If license_info_from_snippet
is None
this method is called up unnecessarily. I would suggest to first check in parse_snippet
if there is a value and if not don't call the method. This applies for all the fields that are mandatory now. I know that this hasn't been done before, so for e.g. packages this would apply as well. I'm not sure if we should change everything in this PR but mybe we could at least start with the parts that are touched here anyway.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree that the None
check is quite hidden there but as I only copied this behaviour from already established cases I propose to leave this refactoring to the big restructure that is coming to the parsers.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tend to agree to leave larger refactorings (that should be applied to most of the parser in this case) for later
Signed-off-by: Armin Tänzer <armin.taenzer@tngtech.com>
41bfd50
to
d6e1df8
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks!
@@ -658,7 +658,7 @@ def parse_snippet_license_info_from_snippet(self, license_info_from_snippet): | |||
self.value_error("SNIPPET_LIC_INFO", lic_in_snippet) | |||
else: | |||
self.value_error("SNIPPET_LIC_INFO", lic_in_snippet) | |||
else: | |||
elif license_info_from_snippet is not None: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tend to agree to leave larger refactorings (that should be applied to most of the parser in this case) for later
fixes #272
only checked and adapted the jsonyamlxml workflow