-
Notifications
You must be signed in to change notification settings - Fork 137
introduce message handler #173
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
hi @pombredanne I cannot find a open source oss managment tool were you are not involved :) |
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.
Thanks! See some comments for your consideration.
# messages = self.validate_annotations(messages) | ||
# messages = self.validate_relationships(messages) | ||
|
||
if isinstance(messages, list): |
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.
Here you are overriding entirely the messages argument, right? What does this mean then? What's the impact?
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 think this was in order to keep API compatibility. but this may not make much sense as there might not be that many users we want to avoid braking.
I'll change that to an explicit error
spdx/document.py
Outdated
self.validate_reviews(messages) | ||
self.validate_snippet(messages) | ||
# self.validate_annotations(messages) | ||
# self.validate_relationships(messages) |
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.
Let's get rid of dead commented code
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.
well, I think it does not make much sense to have it commented out. was introduced here: 209a925
spdx/relationship.py
Outdated
] | ||
return messages | ||
) | ||
return False |
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.
Could we instead always return ErrorMessages? here and elsewhere
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.
The docstring is different for that task, we are supposed to return True if the relationship is valid. I'll fix and change docstrings
Signed-off-by: Pierre Tardy <pierre.tardy@renault.com>
Signed-off-by: Pierre Tardy <pierre.tardy@renault.com>
Signed-off-by: Pierre Tardy <pierre.tardy@renault.com>
Signed-off-by: Pierre Tardy <pierre.tardy@renault.com>
Signed-off-by: Pierre Tardy <pierre.tardy@renault.com>
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.... Thank you!
There are a lot of
in this codebase..
This is not very pythonic, and does not contain much context information, which makes it difficult to understand what part of the 8MB spdx dump file generated by proprietary tool is not recognised.
This is a first simple code to start discussion on the problematic.
Will integrate in the rest of the code when we agree on the API.