Upgrade the 'Document' class to the SPDX 2.1 specification #71
Conversation
pombredanne
left a comment
pombredanne
left a comment
There was a problem hiding this comment.
Looking good... See my comments for a minor change.
As for the validation IMHO you could work out on a cleanup of the way validation works (and how validation messages should be returned rather than a list passed around.)
| Represent an SPDX document with these fields: | ||
| - version: Spec version. Mandatory, one - Type: Version. | ||
| - data_license: SPDX-Metadata license. Mandatory, one. Type: License. | ||
| - name: Name of the document. Mandatory, one. Type: str. |
There was a problem hiding this comment.
Do we really need to make this mandatory?
There was a problem hiding this comment.
I had referred to https://spdx.org/sites/cpstandard/files/pages/files/spdxversion2.1.pdf#page=11 where the Cardinality is Mandatory. So, what should we do ?
| - version: Spec version. Mandatory, one - Type: Version. | ||
| - data_license: SPDX-Metadata license. Mandatory, one. Type: License. | ||
| - name: Name of the document. Mandatory, one. Type: str. | ||
| - spdx_id: SPDX Identifier for the document to refer to itself in |
There was a problem hiding this comment.
Do we really need to make this mandatory too?
There was a problem hiding this comment.
I had referred to https://spdx.org/sites/cpstandard/files/pages/files/spdxversion2.1.pdf#page=10 where the Cardinality is Mandatory. So, what should we do ?
| - ext_document_references: External SPDX documents referenced within the | ||
| given SPDX document. Optional, one or many. Type: ExternalDocumentRef | ||
| - comment: Comments on the SPDX file, optional one. Type: str | ||
| - namespace: SPDX document specific namespace. Mandatory, one. Type: str |
There was a problem hiding this comment.
Do we really need to make this mandatory again?
Note that your doc does not seem to be what happens: this does not seem mandatory in the code.
There was a problem hiding this comment.
def validate_namespace(self, messages=None):
# FIXME: messages should be returned
messages = messages if messages is not None else []
if self.namespace is None:
messages.append('Document has no namespace.')
return False
else:
return True
has been defined which will throw an error if the namespace is not defined :)
Do you want me to add additional tests ?
|
|
||
| return (self.validate_version(messages) | ||
| and self.validate_data_lics(messages) | ||
| and self.validate_name(messages) |
There was a problem hiding this comment.
we likely want the validation to be strict or not.... Note that this PR can still go through and this can be fixed afterwards
There was a problem hiding this comment.
I'm planning to handle the validation part after Phase 1 ? Is that okay ?
spdx/external_document_ref.py
Outdated
| @@ -0,0 +1,89 @@ | |||
|
|
|||
There was a problem hiding this comment.
@pombredanne Do you want me to add this in document.py itself ?
yash-nisar
force-pushed
the
doc-spec-2.1
branch
from
7a1ce7a to
6c81f39
Compare
|
This needs to be updated to resolve merge conflicts. |
yash-nisar
force-pushed
the
doc-spec-2.1
branch
from
6c81f39 to
396df87
Compare
Signed-off-by: Yash Nisar <[email protected]>
Signed-off-by: Yash Nisar <[email protected]>
Signed-off-by: Yash Nisar <[email protected]>
Signed-off-by: Yash Nisar <[email protected]>
yash-nisar
force-pushed
the
doc-spec-2.1
branch
from
396df87 to
e9fb88b
Compare
|
@pombredanne Merge conflicts resolved. Wanna review this one last time ? |
pombredanne
left a comment
pombredanne
left a comment
There was a problem hiding this comment.
I made a few comments that could be addressed afterwards. Using name like checksum consistently throughout is one of them.
Looking good otherwise and merging
| - external_document_id: A unique string containing letters, numbers, '.', | ||
| '-' or '+'. | ||
| - spdx_document_uri: The unique ID of the SPDX document being referenced. | ||
| - check_sum: The checksum of the referenced SPDX document. |
There was a problem hiding this comment.
Why check_sum instead of checksum?
| from spdx import config | ||
|
|
||
|
|
||
| @total_ordering |
|
|
||
| return (self.validate_ext_doc_id(messages) and | ||
| self.validate_spdx_doc_uri(messages) and | ||
| self.validate_chksum(messages) |
There was a problem hiding this comment.
being consistent would be nice: use checksum everywhere?
|
|
||
| if self.external_document_id: | ||
| return True | ||
| else: |
There was a problem hiding this comment.
This else is not needed.... but this is minor.
| Represent an SPDX document with these fields: | ||
| - version: Spec version. Mandatory, one - Type: Version. | ||
| - data_license: SPDX-Metadata license. Mandatory, one. Type: License. | ||
| - name: Name of the document. Mandatory, one. Type: str. |
| - ext_document_references: External SPDX documents referenced within the | ||
| given SPDX document. Optional, one or many. Type: ExternalDocumentRef | ||
| - comment: Comments on the SPDX file, optional one. Type: str | ||
| - namespace: SPDX document specific namespace. Mandatory, one. Type: str |
|
|
||
| return (self.validate_version(messages) | ||
| and self.validate_data_lics(messages) | ||
| and self.validate_name(messages) |
|
|
||
| def validate_ext_document_references(self, messages=None): | ||
| # FIXME: messages should be returned | ||
| messages = messages if messages is not None else [] |
There was a problem hiding this comment.
could a simpler messages = messages or [] be enough here and elsewhere?
2 participants
No description provided.