-
Notifications
You must be signed in to change notification settings - Fork 135
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
[issue-307] implement validation #371
[issue-307] implement validation #371
Conversation
Signed-off-by: Armin Tänzer <armin.taenzer@tngtech.com>
Signed-off-by: Armin Tänzer <armin.taenzer@tngtech.com>
Signed-off-by: Armin Tänzer <armin.taenzer@tngtech.com>
Signed-off-by: Armin Tänzer <armin.taenzer@tngtech.com>
Signed-off-by: Armin Tänzer <armin.taenzer@tngtech.com>
Signed-off-by: Armin Tänzer <armin.taenzer@tngtech.com>
Signed-off-by: Armin Tänzer <armin.taenzer@tngtech.com>
Signed-off-by: Armin Tänzer <armin.taenzer@tngtech.com>
Signed-off-by: Armin Tänzer <armin.taenzer@tngtech.com>
Signed-off-by: Armin Tänzer <armin.taenzer@tngtech.com>
external document references are not implemented correctly here, yet Signed-off-by: Armin Tänzer <armin.taenzer@tngtech.com>
…alidator.py Signed-off-by: Armin Tänzer <armin.taenzer@tngtech.com>
Signed-off-by: Armin Tänzer <armin.taenzer@tngtech.com>
fb1cbfa
to
c23367f
Compare
Signed-off-by: Armin Tänzer <armin.taenzer@tngtech.com>
c23367f
to
9770e7e
Compare
Signed-off-by: Armin Tänzer <armin.taenzer@tngtech.com>
Signed-off-by: Armin Tänzer <armin.taenzer@tngtech.com>
Signed-off-by: Armin Tänzer <armin.taenzer@tngtech.com>
Signed-off-by: Armin Tänzer <armin.taenzer@tngtech.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.
Thanks for all the effort and especially dealing with the whole spec to get all validations, that must have been quite a lot! I have not dealt with the spec in such detail, but I think that you managed to implement all possible validations (except for the open issues of course), congrats on that! 🥇
I left quite a lot comments but most of them deal with double quotes or unused imports. I also left a comment about the architecture what we possibly should discuss. I have to admit: The more I looked into your code, the more unsure i became whether your way isn't the most suitable after all.
Signed-off-by: Armin Tänzer <armin.taenzer@tngtech.com>
45cad5c
to
43f7e01
Compare
Signed-off-by: Armin Tänzer <armin.taenzer@tngtech.com>
Signed-off-by: Armin Tänzer <armin.taenzer@tngtech.com>
Signed-off-by: Armin Tänzer <armin.taenzer@tngtech.com>
Signed-off-by: Armin Tänzer <armin.taenzer@tngtech.com>
Signed-off-by: Armin Tänzer <armin.taenzer@tngtech.com>
Signed-off-by: Armin Tänzer <armin.taenzer@tngtech.com>
(will be properly implemented in test_spdx_id_validators.py) Signed-off-by: Armin Tänzer <armin.taenzer@tngtech.com>
Signed-off-by: Armin Tänzer <armin.taenzer@tngtech.com>
Signed-off-by: Armin Tänzer <armin.taenzer@tngtech.com>
…ually Signed-off-by: Armin Tänzer <armin.taenzer@tngtech.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.
Looks very good to me, thanks for addressing the other comments! I have only a few small remarks left.
One remark concerning usage: To test the validation I used the following code
test_pack = Package(spdx_id="spdxid", name="jusst a test", download_location="I have to put this here")
validate_package(test_pack)
and got an error Missing context
. As ValidationContext
can be instantiated without further parameter, I was wondering if we can make this parameter optional for all validation-methods. What do you think?
Edit: We should also add a license header to the new files.
…alls of packages, files and snippets Signed-off-by: Armin Tänzer <armin.taenzer@tngtech.com>
Signed-off-by: Armin Tänzer <armin.taenzer@tngtech.com>
Signed-off-by: Armin Tänzer <armin.taenzer@tngtech.com>
Signed-off-by: Armin Tänzer <armin.taenzer@tngtech.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.
Looks good to me, thank you very much for addressing all comments! 👍
will fix #307