Return messages instead of modifying list spdx#50

We now return messages from each `validation` function
instead of modifying lists in place. It is preferrable to
have side-effect free functions, so in this case, return a
new list with the element modified. There are a few rare
corner use cases where in place changes can make sense,
but 99% of the times, it a bad idea and can trigger any
kind of weird, byzantine and hard to debug bugs.

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

spdx/creationinfo.py

@@ -152,32 +152,24 @@ def created_iso_format(self):
     def has_comment(self):
         return self.comment is not None
 
-    def validate(self, messages=None):
+    def validate(self, messages):
         """Returns True if the fields are valid according to the SPDX standard.
         Appends user friendly messages to the messages parameter.
         """
-        # FIXME: messages should be returned
-        messages = messages if messages is not None else []
+        messages = self.validate_creators(messages)
+        messages = self.validate_created(messages)
 
-        return (self.validate_creators(messages)
-            and self.validate_created(messages))
+        return messages
 
-    def validate_creators(self, messages=None):
-        # FIXME: messages should be returned
-        messages = messages if messages is not None else []
+    def validate_creators(self, messages):
+        if len(self.creators) == 0:
+            messages = messages + [
+                'No creators defined, must have at least one.']
 
-        if len(self.creators) != 0:
-            return True
-        else:
-            messages.append('No creators defined, must have at least one.')
-            return False
+        return messages
 
-    def validate_created(self, messages=None):
-        # FIXME: messages should be returned
-        messages = messages if messages is not None else []
+    def validate_created(self, messages):
+        if self.created is None:
+            messages = messages + ['Creation info missing created date.']
 
-        if self.created is not None:
-            return True
-        else:
-            messages.append('Creation info missing created date.')
-            return False
+        return messages

spdx/document.py

@@ -173,15 +173,11 @@ def __lt__(self, other):
     def add_xref(self, ref):
         self.cross_ref.append(ref)
 
-    def validate(self, messages=None):
-        # FIXME: messages should be returned
-        messages = messages if messages is not None else []
-
+    def validate(self, messages):
         if self.text is None:
-            messages.append('ExtractedLicense text can not be None')
-            return False
-        else:
-            return True
+            messages = messages + ['ExtractedLicense text can not be None']
+
+        return messages
 
 
 class Document(object):
@@ -227,87 +223,65 @@ def files(self, value):
     def has_comment(self):
         return self.comment is not None
 
-    def validate(self, messages=None):
+    def validate(self, messages):
         """
         Validate all fields of the document and update the
         messages list with user friendly error messages for display.
         """
-        # FIXME: messages should be returned
-        messages = messages if messages is not None else []
+        messages = self.validate_version(messages)
+        messages = self.validate_data_lics(messages)
+        messages = self.validate_creation_info(messages)
+        messages = self.validate_package(messages)
+        messages = self.validate_extracted_licenses(messages)
+        messages = self.validate_reviews(messages)
 
-        return (self.validate_version(messages)
-            and self.validate_data_lics(messages)
-            and self.validate_creation_info(messages)
-            and self.validate_package(messages)
-            and self.validate_extracted_licenses(messages)
-            and self.validate_reviews(messages)
-        )
-
-    def validate_version(self, messages=None):
-        # FIXME: messages should be returned
-        messages = messages if messages is not None else []
+        return messages
 
+    def validate_version(self, messages):
         if self.version is None:
-            messages.append('Document has no version.')
-            return False
-        else:
-            return True
+            messages = messages + ['Document has no version.']
 
-    def validate_data_lics(self, messages=None):
-        # FIXME: messages should be returned
-        messages = messages if messages is not None else []
+        return messages
 
+    def validate_data_lics(self, messages):
         if self.data_license is None:
-            messages.append('Document has no data license.')
-            return False
+            messages = messages + ['Document has no data license.']
 
-        if self.data_license.identifier == 'CC0-1.0':
-            return True
-        else:
-            # FIXME: REALLY? what if someone wants to use something else?
-            messages.append('Document data license must be CC0-1.0.')
-            return False
+        # FIXME: REALLY? what if someone wants to use something else?
+        if self.data_license.identifier != 'CC0-1.0':
+            messages = messages + ['Document data license must be CC0-1.0.']
 
-    def validate_reviews(self, messages=None):
-        # FIXME: messages should be returned
-        messages = messages if messages is not None else []
+        return messages
 
-        valid = True
+    def validate_reviews(self, messages):
         for review in self.reviews:
-            valid = review.validate(messages) and valid
-        return valid
+            messages = review.validate(messages)
 
-    def validate_creation_info(self, messages=None):
-        # FIXME: messages should be returned
-        messages = messages if messages is not None else []
+        return messages
 
+    def validate_creation_info(self, messages):
         if self.creation_info is not None:
-            return self.creation_info.validate(messages)
+            messages = self.creation_info.validate(messages)
         else:
-            messages.append('Document has no creation information.')
-            return False
+            messages = messages + ['Document has no creation information.']
 
-    def validate_package(self, messages=None):
-        # FIXME: messages should be returned
-        messages = messages if messages is not None else []
+        return messages
 
+    def validate_package(self, messages):
         if self.package is not None:
-            return self.package.validate(messages)
+            messages = self.package.validate(messages)
         else:
-            messages.append('Document has no package.')
-            return False
+            messages = messages + ['Document has no package.']
 
-    def validate_extracted_licenses(self, messages=None):
-        # FIXME: messages should be returned
-        messages = messages if messages is not None else []
+        return messages
 
-        valid = True
+    def validate_extracted_licenses(self, messages):
         for lic in self.extracted_licenses:
             if isinstance(lic, ExtractedLicense):
-                valid = lic.validate(messages) and valid
+                messages = lic.validate(messages)
             else:
-                messages.append(
+                messages = messages + [
                     'Document extracted licenses must be of type '
-                    'spdx.document.ExtractedLicense and not ' + type(lic))
-                valid = False
-        return valid
+                    'spdx.document.ExtractedLicense and not ' + type(lic)
+                ]
+        return messages

spdx/file.py

@@ -93,90 +93,82 @@ def add_artifact(self, symbol, value):
         artifact = getattr(self, symbol)
         artifact.append(value)
 
-    def validate(self, messages=None):
+    def validate(self, messages):
         """Validates the fields and appends user friendly messages
         to messages parameter if there are errors.
         """
-        # FIXME: messages should be returned
-        messages = messages if messages is not None else []
+        messages = self.validate_concluded_license(messages)
+        messages = self.validate_type(messages)
+        messages = self.validate_chksum(messages)
+        messages = self.validate_licenses_in_file(messages)
+        messages = self.validate_copyright(messages)
+        messages = self.validate_artifacts(messages)
 
-        return (self.validate_concluded_license(messages)
-            and self.validate_type(messages)
-            and self.validate_chksum(messages)
-            and self.validate_licenses_in_file(messages)
-            and self.validate_copyright(messages)
-            and self.validate_artifacts(messages))
+        return messages
 
-    def validate_copyright(self, messages=None):
-        # FIXME: messages should be returned
-        messages = messages if messages is not None else []
-
-        if isinstance(
+    def validate_copyright(self, messages):
+        if not isinstance(
             self.copyright,
-            (six.string_types, six.text_type, utils.NoAssert, utils.SPDXNone)):
-            return True
-        else:
-            messages.append('File copyright must be str or unicode or utils.NoAssert or utils.SPDXNone')
-            return False
+            (six.string_types, six.text_type, utils.NoAssert, utils.SPDXNone)
+        ):
+            messages = messages + [
+                'File copyright must be str or unicode or '
+                'utils.NoAssert or utils.SPDXNone'
+            ]
 
-    def validate_artifacts(self, messages=None):
-        # FIXME: messages should be returned
-        messages = messages if messages is not None else []
+        return messages
 
-        if (len(self.artifact_of_project_home) >=
-            max(len(self.artifact_of_project_uri), len(self.artifact_of_project_name))):
-            return True
-        else:
-            messages.append('File must have as much artifact of project as uri or homepage')
-            return False
+    def validate_artifacts(self, messages):
+        if (len(self.artifact_of_project_home) <
+            max(len(self.artifact_of_project_uri),
+                len(self.artifact_of_project_name))):
+            messages = messages + [
+                'File must have as much artifact of project as uri or homepage']
 
-    def validate_licenses_in_file(self, messages=None):
-        # FIXME: messages should be returned
-        messages = messages if messages is not None else []
+        return messages
 
+    def validate_licenses_in_file(self, messages):
         # FIXME: what are we testing the length of a list? or?
         if len(self.licenses_in_file) == 0:
-            messages.append('File must have at least one license in file.')
-            return False
-        else:
-            return True
+            messages = messages + [
+                'File must have at least one license in file.'
+            ]
 
-    def validate_concluded_license(self, messages=None):
-        # FIXME: messages should be returned
-        messages = messages if messages is not None else []
+        return messages
 
-        if isinstance(self.conc_lics, (document.License, utils.NoAssert,
-                                       utils.SPDXNone)):
-            return True
-        else:
-            messages.append(
+    def validate_concluded_license(self, messages):
+        # FIXME: use isinstance instead??
+        if not isinstance(self.conc_lics, (document.License, utils.NoAssert,
+                                           utils.SPDXNone)):
+            messages = messages + [
                 'File concluded license must be one of '
-                'document.License, utils.NoAssert or utils.SPDXNone')
-            return False
-
-    def validate_type(self, messages=None):
-        # FIXME: messages should be returned
-        messages = messages if messages is not None else []
-
-        if self.type in [None, FileType.SOURCE, FileType.OTHER, FileType.BINARY, FileType.ARCHIVE]:
-            return True
+                'document.License, utils.NoAssert or utils.SPDXNone'
+            ]
+
+        return messages
+
+    def validate_type(self, messages):
+        if self.type not in [
+            None, FileType.SOURCE, FileType.OTHER, FileType.BINARY,
+            FileType.ARCHIVE
+        ]:
+            messages = messages + [
+                'File type must be one of the constants defined in '
+                'class spdx.file.FileType'
+            ]
+
+        return messages
+
+    def validate_chksum(self, messages):
+        if not isinstance(self.chk_sum, checksum.Algorithm):
+            messages = messages + [
+                'File checksum must be instance of spdx.checksum.Algorithm'
+            ]
         else:
-            messages.append('File type must be one of the constants defined in class spdx.file.FileType')
-            return False
-
-    def validate_chksum(self, messages=None):
-        # FIXME: messages should be returned
-        messages = messages if messages is not None else []
-
-        if isinstance(self.chk_sum, checksum.Algorithm):
-            if self.chk_sum.identifier == 'SHA1':
-                return True
-            else:
-                messages.append('File checksum algorithm must be SHA1')
-                return False
-        else:
-            messages.append('File checksum must be instance of spdx.checksum.Algorithm')
-            return False
+            if not self.chk_sum.identifier == 'SHA1':
+                messages = messages + ['File checksum algorithm must be SHA1']
+
+        return messages
 
     def calc_chksum(self):
         BUFFER_SIZE = 65536