merge the fixes from #349

Signed-off-by: Chin Yeung Li <[email protected]>

Author: chinyeungli

docs/CHANGELOG.rst

@@ -3,6 +3,10 @@
     Release x.x.x
 
     * Update the list of common license keys
+    * New UrlListField introduced for list of urls
+    * The UrlField is now only taking single URL value
+    * The owner is now a StringField instead of ListField
+    * Format the ordering of the genreated ABOUT file (See https://github.com/nexB/aboutcode-toolkit/issues/349#issuecomment-438871444)
 
 2018-10-23
 

src/attributecode/gen.py

@@ -302,7 +302,7 @@ def generate(location, base_dir, license_notice_text_location=None,
                             about.license_name.present = True
 
             # Write the ABOUT files
-            about.dump(dump_loc, with_empty=with_empty, with_absent=with_absent)
+            about.dump(dump_loc, use_mapping=use_mapping, mapping_file=mapping_file, with_empty=with_empty, with_absent=with_absent)
             for e in not_exist_errors:
                 errors.append(Error(ERROR, e))
         except Exception as e:

src/attributecode/model.py

@@ -344,20 +344,19 @@ def __eq__(self, other):
         if sval == oval:
             return True
 
-
-class UrlField(ListField):
+class UrlListField(ListField):
     """
     A URL field. The validated value is a list of URLs.
     """
     def _validate(self, *args, **kwargs):
         """
         Check that URLs are valid. Return a list of errors.
         """
-        errors = super(UrlField, self)._validate(*args, ** kwargs)
-        for url in self.value:
+        errors = super(UrlListField, self)._validate(*args, ** kwargs)
+        name = self.name
+        val = self.value
+        for url in val:
             if not self.is_valid_url(url):
-                name = self.name
-                val = self.value
                 msg = (u'Field %(name)s: Invalid URL: %(val)s' % locals())
                 errors.append(Error(WARNING, msg))
         return errors
@@ -372,6 +371,32 @@ def is_valid_url(url):
         return valid
 
 
+class UrlField(StringField):
+    """
+    A URL field. The validated value is a URL.
+    """
+    def _validate(self, *args, **kwargs):
+        """
+        Check that URL is valid. Return a list of errors.
+        """
+        errors = super(UrlField, self)._validate(*args, ** kwargs)
+        name = self.name
+        val = self.value
+        if not self.is_valid_url(val):
+            msg = (u'Field %(name)s: Invalid URL: %(val)s' % locals())
+            errors.append(Error(WARNING, msg))
+        return errors
+
+    @staticmethod
+    def is_valid_url(url):
+        """
+        Return True if a URL is valid.
+        """
+        scheme, netloc, _path, _p, _q, _frg = urlparse(url)
+        valid = scheme in ('http', 'https', 'ftp') and netloc
+        return valid
+
+
 class PathField(ListField):
     """
     A field pointing to one or more paths relative to the ABOUT file location.
@@ -692,7 +717,7 @@ def create_fields(self):
             ('license_key', ListField()),
             ('license_name', ListField()),
             ('license_file', FileTextField()),
-            ('license_url', UrlField()),
+            ('license_url', UrlListField()),
             ('copyright', StringField()),
             ('notice_file', FileTextField()),
             ('notice_url', UrlField()),
@@ -704,7 +729,7 @@ def create_fields(self):
 
             ('changelog_file', FileTextField()),
 
-            ('owner', ListField()),
+            ('owner', StringField()),
             ('owner_url', UrlField()),
             ('contact', ListField()),
             ('author', ListField()),
@@ -1066,7 +1091,7 @@ def load_dict(self, fields_dict, base_dir, running_inventory=False,
         return errors
 
 
-    def dumps(self, with_absent=False, with_empty=True):
+    def dumps(self, use_mapping=False, mapping_file=False, with_absent=False, with_empty=True):
         """
         Return self as a formatted ABOUT string.
         If with_absent, include absent (not present) fields.
@@ -1113,9 +1138,10 @@ def dumps(self, with_absent=False, with_empty=True):
             if lic_group[3]:
                 lic_dict['url'] = lic_group[3] 
             about_data.setdefault('licenses', []).append(lic_dict)
-        return saneyaml.dump(about_data)
+        formatted_about_data = util.format_output(about_data, use_mapping, mapping_file)
+        return saneyaml.dump(formatted_about_data)
 
-    def dump(self, location, with_absent=False, with_empty=True):
+    def dump(self, location, use_mapping=False, mapping_file=False, with_absent=False, with_empty=True):
         """
         Write formatted ABOUT representation of self to location.
         If with_absent, include absent (not present) fields.
@@ -1135,7 +1161,7 @@ def dump(self, location, with_absent=False, with_empty=True):
         if on_windows:
             about_file_path = add_unc(about_file_path)
         with codecs.open(about_file_path, mode='wb', encoding='utf-8') as dumped:
-            dumped.write(self.dumps(with_absent, with_empty))
+            dumped.write(self.dumps(use_mapping, mapping_file, with_absent, with_empty))
 
     def dump_lic(self, location, license_dict):
         """

src/attributecode/util.py

@@ -330,7 +330,7 @@ def get_mapping(location=None):
     if not os.path.exists(location):
         return {}
 
-    mapping = {}
+    mapping = collections.OrderedDict()
     try:
         with open(location) as mapping_file:
             for line in mapping_file:
@@ -412,6 +412,52 @@ def apply_mapping(abouts, alternate_mapping=None):
         mapped_abouts.append(mapped_about)
     return mapped_abouts
 
+def get_mapping_key_order(mapping_file):
+    """
+    Get the mapping key order and return as a list
+    """
+    if mapping_file:
+        mapping = get_mapping(mapping_file)
+    else:
+        mapping = get_mapping()
+    return mapping.keys()
+
+def format_output(about_data, use_mapping, mapping_file):
+    """
+    Convert the about_data dictionary to an ordered dictionary for saneyaml.dump()
+    The ordering should be:
+
+    about_resource
+    name
+    version <-- if any
+    and the rest is the order from the mapping.config file (if any); otherwise alphabetical order.
+    """
+    mapping_key_order = []
+    if use_mapping or mapping_file:
+        mapping_key_order = get_mapping_key_order(mapping_file)
+    priority_keys = [u'about_resource', u'name', u'version']
+    about_data_keys = []
+    order_dict = collections.OrderedDict()
+    for key in about_data:
+        about_data_keys.append(key)
+    if u'about_resource' in about_data_keys:
+        order_dict['about_resource'] = about_data['about_resource']
+    if u'name' in about_data_keys:
+        order_dict['name'] = about_data['name']
+    if u'version' in about_data_keys:
+        order_dict['version'] = about_data['version']
+    if not mapping_key_order:
+        for other_key in sorted(about_data_keys):
+            if not other_key in priority_keys:
+                order_dict[other_key] = about_data[other_key]
+    else:
+        for key in mapping_key_order:
+            if not key in priority_keys and key in about_data_keys:
+                order_dict[key] = about_data[key]
+        for other_key in sorted(about_data_keys):
+            if not other_key in priority_keys and not other_key in mapping_key_order:
+                order_dict[other_key] = about_data[other_key]
+    return order_dict
 
 def get_about_file_path(location, use_mapping=False, mapping_file=None):
     """

tests/test_gen.py

@@ -62,12 +62,12 @@ def test_load_inventory(self):
         assert expected_errors == errors
 
         expected = [u'about_resource: .\n'
+                    u'name: AboutCode\n'
+                    u'version: 0.11.0\n'
                     u'description: |-\n'
                     u'    multi\n'
-                    u'    line\n'
-                    u'name: AboutCode\n'
-                    u'version: 0.11.0\n']
-        result = [a.dumps(with_absent=False, with_empty=False)
+                    u'    line\n']
+        result = [a.dumps(use_mapping=False, mapping_file=False, with_absent=False, with_empty=False)
                         for a in abouts]
         assert expected == result
 
@@ -86,15 +86,15 @@ def test_load_inventory_with_mapping(self):
         assert sorted(expected_errors) == sorted(errors)
 
         expected = [u'about_resource: .\n'
+                    u'name: AboutCode\n'
+                    u'version: 0.11.0\n'
                     u'copyright: Copyright (c) nexB, Inc.\n'
+                    u'resource: this.ABOUT\n'
                     u'description: |-\n'
                     u'    multi\n'
                     u'    line\n'
-                    u'name: AboutCode\n'
-                    u'resource: this.ABOUT\n'
-                    u'version: 0.11.0\n'
                     ]
-        result = [a.dumps(with_absent=False, with_empty=False)
+        result = [a.dumps(use_mapping, mapping_file=False, with_absent=False, with_empty=False)
                         for a in abouts]
         assert expected == result
 
@@ -148,15 +148,15 @@ def test_generate(self):
         assert msg1 in errors[0].message
         assert msg2 in errors[1].message
 
-        in_mem_result = [a.dumps(with_absent=False, with_empty=False)
+        in_mem_result = [a.dumps(use_mapping=False, mapping_file=False, with_absent=False, with_empty=False)
                         for a in abouts][0]
         expected = (u'about_resource: .\n'
+                    u'name: AboutCode\n'
+                    u'version: 0.11.0\n'
                     u'about_resource_path: .\n'
                     u'description: |-\n'
                     u'    multi\n'
-                    u'    line\n'
-                    u'name: AboutCode\n'
-                    u'version: 0.11.0\n')
+                    u'    line\n')
         assert expected == in_mem_result
 
     def test_generate_not_overwrite_original_license_file(self):
@@ -167,14 +167,14 @@ def test_generate_not_overwrite_original_license_file(self):
 
         errors, abouts = gen.generate(location, base_dir, license_notice_text_location, fetch_license)
 
-        in_mem_result = [a.dumps(with_absent=False, with_empty=False)
+        in_mem_result = [a.dumps(use_mapping=False, mapping_file=False, with_absent=False, with_empty=False)
                         for a in abouts][0]
         expected = (u'about_resource: .\n'
+                    u'name: AboutCode\n'
+                    u'version: 0.11.0\n'
                     u'about_resource_path: .\n'
                     u'licenses:\n'
-                    u'    -   file: this.LICENSE\n'
-                    u'name: AboutCode\n'
-                    u'version: 0.11.0\n')
+                    u'    -   file: this.LICENSE\n')
         assert expected == in_mem_result
 
     def test_deduplicate(self):

tests/test_model.py

@@ -193,12 +193,15 @@ def test_PathField_contains_dict_after_validate(self):
                           ]
         self.check_validate(field_class, value, expected, expected_errors)
 
+    """
+    UrlField no longer become a list.
+    If a list is wanted, use UrlListField instead.
     def test_UrlField_contains_list_after_validate(self):
         value = 'http://some.com/url'
         field_class = model.UrlField
         expected = [value]
         self.check_validate(field_class, value, expected, expected_errors=[])
-
+    """
     def test_SingleLineField_has_errors_if_multiline(self):
         value = '''line1
         line2'''
@@ -704,31 +707,29 @@ def test_About_dumps(self):
         assert [] == a.errors
 
         expected = u'''about_resource: .
+name: AboutCode
+version: 0.11.0
+copyright: Copyright (c) 2013-2014 nexB Inc.
+license_expression: apache-2.0
 author:
     - Jillian Daguil
     - Chin Yeung Li
     - Philippe Ombredanne
     - Thomas Druez
-copyright: Copyright (c) 2013-2014 nexB Inc.
 description: |-
     AboutCode is a tool
     to process ABOUT files.
     An ABOUT file is a file.
-homepage_url:
-    - http://dejacode.org
-license_expression: apache-2.0
+homepage_url: http://dejacode.org
 licenses:
     -   file: apache-2.0.LICENSE
         key: apache-2.0
-name: AboutCode
 notice_file: NOTICE
-owner:
-    - nexB Inc.
+owner: nexB Inc.
 vcs_repository: https://github.com/dejacode/about-code-tool.git
 vcs_tool: git
-version: 0.11.0
 '''
-        result = a.dumps()
+        result = a.dumps(use_mapping=True)
         assert expected == result
 
     # We do not support with_absent and with_empty staring in version 3.2.0.
@@ -810,7 +811,7 @@ def test_About_dumps_with_different_boolean_value(self):
 modified: yes
 '''
 
-        result = a.dumps()
+        result = a.dumps(use_mapping=False, mapping_file=False)
         assert set(expected) == set(result)
 
 
@@ -826,7 +827,7 @@ def test_About_dumps_does_not_dump_present__empty_with_absent_False(self):
 name: AboutCode
 version: 0.11.0
 '''
-        result = a.dumps(with_absent=False, with_empty=False)
+        result = a.dumps(use_mapping=False, mapping_file=False, with_absent=False, with_empty=False)
         assert expected == result
 
     def test_About_as_dict_contains_special_paths(self):
@@ -927,7 +928,7 @@ def test_About_as_dict_with_nothing(self):
                     'license_key': [u'apache-2.0'],
                     'license_expression': u'apache-2.0',
                     'name': u'AboutCode',
-                    'owner': [u'nexB Inc.']}
+                    'owner': u'nexB Inc.'}
         result = a.as_dict(with_paths=False,
                            with_empty=False,
                            with_absent=False)
@@ -939,7 +940,7 @@ def test_load_dump_is_idempotent(self):
         a = model.About()
         a.load(test_file)
         dumped_file = get_temp_file('that.ABOUT')
-        a.dump(dumped_file, with_absent=False, with_empty=False)
+        a.dump(dumped_file, use_mapping=False, mapping_file=False, with_absent=False, with_empty=False)
 
         expected = get_unicode_content(test_file).splitlines()
         result = get_unicode_content(dumped_file).splitlines()
@@ -987,7 +988,7 @@ def test_as_dict_load_dict_is_idempotent(self):
                  'description': u'AboutCode is a tool\nfor files.',
                  'license_expression': u'apache-2.0',
                  'name': u'AboutCode',
-                 'owner': [u'nexB Inc.']}
+                 'owner': u'nexB Inc.'}
 
         a = model.About()
         base_dir = 'some_dir'
@@ -1002,10 +1003,10 @@ def test_load_dict_handles_field_validation_correctly(self):
                 u'author': [u'Jillian Daguil, Chin Yeung Li, Philippe Ombredanne, Thomas Druez'],
                 u'copyright': u'Copyright (c) 2013-2014 nexB Inc.',
                 u'description': u'AboutCode is a tool to process ABOUT files. An ABOUT file is a file.',
-                u'homepage_url': [u'http://dejacode.org'],
+                u'homepage_url': u'http://dejacode.org',
                 u'license_expression': u'apache-2.0',
                 u'name': u'AboutCode',
-                u'owner': [u'nexB Inc.'],
+                u'owner': u'nexB Inc.',
                 u'vcs_repository': u'https://github.com/dejacode/about-code-tool.git',
                 u'vcs_tool': u'git',
                 u'version': u'0.11.0'}

tests/test_util.py

@@ -266,6 +266,17 @@ def test_get_relative_path_with_same_path_twice(self):
             result = util.get_relative_path(loc, loc)
             assert expected == result
 
+    def test_get_mapping_key_order(self):
+        expected = ['about_file_path', 'name', 'version', 'copyright', 'license_expression', 'resource']
+        result = util.get_mapping_key_order(mapping_file=False)
+        assert expected == result
+
+    def test_get_mapping_key_order_with_mapping_file(self):
+        expected = ['about_file_path', 'name', 'version', 'description', 'license_expression', 'copyright']
+        test_mapping_file = get_test_loc('mapping_config/mapping.config')
+        result = util.get_mapping_key_order(test_mapping_file)
+        assert expected == result
+
     def test_load_csv_without_mapping(self):
         test_file = get_test_loc('util/about.csv')
         expected = [OrderedDict(