Upgrade the 'Document' class to the SPDX 2.1 specification #71

yash-nisar · 2018-05-25T16:53:35Z

No description provided.

pombredanne

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.)

pombredanne · 2018-05-30T08:49:09Z

spdx/document.py

@@ -189,7 +190,13 @@ class Document(object):
    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.


Do we really need to make this mandatory?

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 ?

leave as is.

pombredanne · 2018-05-30T08:49:20Z

spdx/document.py

@@ -189,7 +190,13 @@ class Document(object):
    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.
+    - spdx_id: SPDX Identifier for the document to refer to itself in


Do we really need to make this mandatory too?

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 ?

pombredanne · 2018-05-30T08:49:32Z

spdx/document.py

    - comment: Comments on the SPDX file, optional one. Type: str
+    - namespace: SPDX document specific namespace. Mandatory, one. Type: str


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.

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 ?

that's ok for now

pombredanne · 2018-05-30T08:51:43Z

spdx/document.py

@@ -237,11 +252,14 @@ def validate(self, messages=None):

        return (self.validate_version(messages)
            and self.validate_data_lics(messages)
+            and self.validate_name(messages)


we likely want the validation to be strict or not.... Note that this PR can still go through and this can be fixed afterwards

I'm planning to handle the validation part after Phase 1 ? Is that okay ?

yash-nisar · 2018-06-17T11:28:15Z

spdx/external_document_ref.py

@@ -0,0 +1,89 @@
+


@pombredanne Do you want me to add this in document.py itself ?

pombredanne · 2018-08-06T21:01:00Z

This needs to be updated to resolve merge conflicts.

Signed-off-by: Yash Nisar <yash.nisar@somaiya.edu>

yash-nisar · 2018-08-07T04:01:14Z

@pombredanne Merge conflicts resolved. Wanna review this one last time ?

pombredanne

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

pombredanne · 2018-08-07T06:57:56Z

spdx/document.py

+    - 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.


Why check_sum instead of checksum?

pombredanne · 2018-08-07T06:58:19Z

spdx/document.py

@@ -19,6 +19,78 @@
 from spdx import config


+@total_ordering


Good usage of this 👍

pombredanne · 2018-08-07T06:59:02Z

spdx/document.py

+
+        return (self.validate_ext_doc_id(messages) and
+                self.validate_spdx_doc_uri(messages) and
+                self.validate_chksum(messages)


being consistent would be nice: use checksum everywhere?

pombredanne · 2018-08-07T06:59:47Z

spdx/document.py

+
+        if self.external_document_id:
+            return True
+        else:


This else is not needed.... but this is minor.

pombredanne · 2018-08-07T07:00:39Z

spdx/document.py

@@ -189,7 +190,13 @@ class Document(object):
    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.


leave as is.

pombredanne · 2018-08-07T07:01:06Z

spdx/document.py

    - comment: Comments on the SPDX file, optional one. Type: str
+    - namespace: SPDX document specific namespace. Mandatory, one. Type: str


that's ok for now

pombredanne · 2018-08-07T07:01:16Z

spdx/document.py

@@ -237,11 +252,14 @@ def validate(self, messages=None):

        return (self.validate_version(messages)
            and self.validate_data_lics(messages)
+            and self.validate_name(messages)


pombredanne · 2018-08-07T07:01:57Z

spdx/document.py

+
+    def validate_ext_document_references(self, messages=None):
+        # FIXME: messages should be returned
+        messages = messages if messages is not None else []


could a simpler messages = messages or [] be enough here and elsewhere?

yash-nisar force-pushed the doc-spec-2.1 branch from ca9fc33 to f50472d Compare May 26, 2018 18:11

pombredanne requested changes May 30, 2018

View reviewed changes

This was referenced May 30, 2018

Add 'namespace' field to the 'Document' class #70

Closed

Add 'spdx_id' field to the 'Document' class #68

Closed

Add 'name' field to the 'Document' class #67

Closed

yash-nisar force-pushed the doc-spec-2.1 branch from f50472d to 045270d Compare June 2, 2018 12:42

yash-nisar mentioned this pull request Jun 2, 2018

Upgrade 'Document Creation Information' according to the 2.1 spec #66

Closed

4 tasks

yash-nisar force-pushed the doc-spec-2.1 branch from 045270d to 7a1ce7a Compare June 5, 2018 13:14

yash-nisar commented Jun 17, 2018

View reviewed changes

yash-nisar force-pushed the doc-spec-2.1 branch from 7a1ce7a to 6c81f39 Compare August 3, 2018 17:14

yash-nisar force-pushed the doc-spec-2.1 branch from 6c81f39 to 396df87 Compare August 7, 2018 03:58

yash-nisar added 4 commits August 7, 2018 11:58

Add 'name' field to the 'Document' class

2048267

Signed-off-by: Yash Nisar <yash.nisar@somaiya.edu>

Add 'spdx_id' field to the 'Document' class

037ffad

Signed-off-by: Yash Nisar <yash.nisar@somaiya.edu>

Add 'namespace' field to the 'Document' class

545d5ba

Signed-off-by: Yash Nisar <yash.nisar@somaiya.edu>

Add ExternalDocumentRef class

e9fb88b

Signed-off-by: Yash Nisar <yash.nisar@somaiya.edu>

yash-nisar force-pushed the doc-spec-2.1 branch from 396df87 to e9fb88b Compare August 7, 2018 03:59

pombredanne approved these changes Aug 7, 2018

View reviewed changes

pombredanne merged commit 0a79680 into spdx:master Aug 7, 2018

yash-nisar deleted the doc-spec-2.1 branch August 8, 2018 09:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Upgrade the 'Document' class to the SPDX 2.1 specification #71

Upgrade the 'Document' class to the SPDX 2.1 specification #71

yash-nisar commented May 25, 2018

pombredanne left a comment

pombredanne May 30, 2018

yash-nisar Jun 1, 2018

pombredanne Aug 7, 2018

pombredanne May 30, 2018

yash-nisar Jun 1, 2018

pombredanne May 30, 2018

yash-nisar Jun 1, 2018

pombredanne Aug 7, 2018

pombredanne May 30, 2018

yash-nisar Jun 1, 2018

pombredanne Aug 7, 2018

yash-nisar Jun 17, 2018

pombredanne Jun 18, 2018

pombredanne commented Aug 6, 2018

yash-nisar commented Aug 7, 2018

pombredanne left a comment

pombredanne Aug 7, 2018

pombredanne Aug 7, 2018

pombredanne Aug 7, 2018

pombredanne Aug 7, 2018

pombredanne Aug 7, 2018

pombredanne Aug 7, 2018

pombredanne Aug 7, 2018

pombredanne Aug 7, 2018

		- comment: Comments on the SPDX file, optional one. Type: str
		- namespace: SPDX document specific namespace. Mandatory, one. Type: str

Upgrade the 'Document' class to the SPDX 2.1 specification #71

Upgrade the 'Document' class to the SPDX 2.1 specification #71

Conversation

yash-nisar commented May 25, 2018

pombredanne left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pombredanne commented Aug 6, 2018

yash-nisar commented Aug 7, 2018

pombredanne left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment