Rename gen --license-notice-text-location #367

 * Rename gen --license-notice-text-location to --reference
 * Renamere license_notice_text_location arg and variable name to
   reference_dir everywhere.
 * Update and refine the command help help for the gen subcommand.

Signed-off-by: Philippe Ombredanne <[email protected]>

Author: pombredanne

src/attributecode/cmd.py

@@ -246,26 +246,19 @@ def inventory(location, output, mapping, mapping_file,
 @click.option('--fetch-license',
     nargs=2,
     type=str,
-    metavar='KEY',
-    help='Fetch license data and texts from a a DejaCode License Library API. '
-         'Create <license>.LICENSE files from the text of each license key '
-         'side-by-side with the generated .ABOUT file. Also enhance the .ABOUT '
-         'file with other data such name and category.\n\n'
-         'The following additional options are required:\n\n'
-         'api_url - URL to the DejaCode License Library API endpoint\n\n'
-         'api_key - DejaCode API key'
-         '\nExample syntax:\n\n'
-         "about gen --fetch-license 'api_url' 'api_key'")
-
-# TODO: this option help and long name is obscure and would need to be refactored
-@click.option('--license-notice-text-location',
-    type=click.Path(exists=True, dir_okay=True, readable=True, resolve_path=True),
-    help="Copy the 'license_file' from the directory to the generated location.")
+    metavar='URL KEY',
+    help='Fetch license data and text files from a DejaCode License Library '
+         'API URL using the API KEY.')
+
+@click.option('--reference',
+    metavar='DIR',
+    type=click.Path(exists=True, file_okay=False, readable=True, resolve_path=True),
+    help='Path to a directory with reference license data and text files.')
 
 @click.option('--mapping',
     is_flag=True,
     help='Use the default file mapping.config (./attributecode/mapping.config) '
-    'with mapping between input keys and ABOUT field names.')
+         'with mapping between input keys and ABOUT field names.')
 
 @click.option('--mapping-file',
     metavar='FILE',
@@ -284,7 +277,7 @@ def inventory(location, output, mapping, mapping_file,
 
 def gen(location, output,
         fetch_license,
-        license_notice_text_location,
+        reference,
         mapping, mapping_file,
         quiet, verbose):
     """
@@ -309,7 +302,7 @@ def gen(location, output,
     errors, abouts = generate_about_files(
         location=location,
         base_dir=output,
-        license_notice_text_location=license_notice_text_location,
+        reference_dir=reference,
         fetch_license=fetch_license,
         mapping_file=mapping_file
     )

src/attributecode/gen.py

@@ -101,22 +101,25 @@ def check_duplicated_about_file_path(inventory_dict):
     return errors
 
 # TODO: this should be either the CSV or the ABOUT files but not both???
-def load_inventory(location, base_dir, license_notice_text_location=None,
-                    mapping_file=None):
+def load_inventory(location, base_dir, reference_dir=None,
+                   mapping_file=None):
     """
     Load the inventory file at `location` for ABOUT and LICENSE files
     stored in the `base_dir`. Return a list of errors and a list of
     About objects validated against the `base_dir`.
-    Optionally use `license_notice_text_location` as the location of
-    license and notice texts.
+
+    Optionally use `reference_dir` as the directory location of
+    reference license and notice files to reuse.
 
     Optionally use mappings for field names if `mapping_file` is provided for
     the CSV format.
     """
     errors = []
     abouts = []
     base_dir = util.to_posix(base_dir)
+    # FIXME: do not mix up CSV and JSON
     if location.endswith('.csv'):
+        # FIXME: this should not be done here.
         dup_cols_err = check_duplicated_columns(location)
         if dup_cols_err:
             errors.extend(dup_cols_err)
@@ -126,6 +129,7 @@ def load_inventory(location, base_dir, license_notice_text_location=None,
         inventory = util.load_json(location)
 
     try:
+        # FIXME: this should not be done here.
         dup_about_paths_err = check_duplicated_about_file_path(inventory)
         if dup_about_paths_err:
             errors.extend(dup_about_paths_err)
@@ -157,6 +161,7 @@ def load_inventory(location, base_dir, license_notice_text_location=None,
                 return errors, abouts
         afp = fields.get(model.About.about_file_path_attr)
 
+        # FIXME: this should not be a failure condition
         if not afp or not afp.strip():
             msg = 'Empty column: %(afp)r. Cannot generate .ABOUT file.' % locals()
             errors.append(Error(ERROR, msg))
@@ -172,7 +177,7 @@ def load_inventory(location, base_dir, license_notice_text_location=None,
             base_dir,
             running_inventory,
             mapping_file,
-            license_notice_text_location,
+            reference_dir,
             with_empty=False
         )
         # 'about_resource' field will be generated during the process.
@@ -184,10 +189,11 @@ def load_inventory(location, base_dir, license_notice_text_location=None,
             if not e in errors:
                 errors.extend(ld_errors)
         abouts.append(about)
+
     return unique(errors), abouts
 
 
-def generate(location, base_dir, license_notice_text_location=None,
+def generate(location, base_dir, reference_dir=None,
              fetch_license=False, policy=None, conf_location=None,
              with_empty=False, with_absent=False, mapping_file=None):
     """
@@ -200,18 +206,20 @@ def generate(location, base_dir, license_notice_text_location=None,
     api_url = ''
     api_key = ''
     gen_license = False
+    # FIXME: use two different arguments: key and url
     # Check if the fetch_license contains valid argument
     if fetch_license:
         # Strip the ' and " for api_url, and api_key from input
         api_url = fetch_license[0].strip("'").strip('"')
         api_key = fetch_license[1].strip("'").strip('"')
         gen_license = True
 
+    # TODO: WHY?
     bdir = to_posix(base_dir)
     errors, abouts = load_inventory(
         location=location,
         base_dir=bdir,
-        license_notice_text_location=license_notice_text_location,
+        reference_dir=reference_dir,
         mapping_file=mapping_file)
 
     if gen_license:

src/attributecode/model.py

@@ -34,9 +34,9 @@
 import os
 # FIXME: why posixpath???
 import posixpath
+import re
 
 import yaml
-import re
 
 from attributecode.util import python2
 
@@ -420,7 +420,7 @@ def _validate(self, *args, **kwargs):
         self.about_file_path = kwargs.get('about_file_path')
         self.running_inventory = kwargs.get('running_inventory')
         self.base_dir = kwargs.get('base_dir')
-        self.license_notice_text_location = kwargs.get('license_notice_text_location')
+        self.reference_dir = kwargs.get('reference_dir')
 
         if self.base_dir:
             self.base_dir = util.to_posix(self.base_dir)
@@ -447,16 +447,16 @@ def _validate(self, *args, **kwargs):
             # the license files, if need to be copied, are located under the path
             # set from the 'license-text-location' option, so the tool should check
             # at the 'license-text-location' instead of the 'base_dir'
-            if not (self.base_dir or self.license_notice_text_location):
+            if not (self.base_dir or self.reference_dir):
                 msg = (u'Field %(name)s: Unable to verify path: %(path)s:'
                        u' No base directory provided' % locals())
                 errors.append(Error(ERROR, msg))
                 location = None
                 paths[path] = location
                 continue
 
-            if self.license_notice_text_location:
-                location = posixpath.join(self.license_notice_text_location, path)
+            if self.reference_dir:
+                location = posixpath.join(self.reference_dir, path)
             else:
                 # The 'about_resource' should be a joined path with
                 # the 'about_file_path' and the 'base_dir
@@ -669,7 +669,7 @@ def __eq__(self, other):
 
 
 def validate_fields(fields, about_file_path, running_inventory, base_dir,
-                    license_notice_text_location=None):
+                    reference_dir=None):
     """
     Validate a sequence of Field objects. Return a list of errors.
     Validation may update the Field objects as needed as a side effect.
@@ -680,7 +680,7 @@ def validate_fields(fields, about_file_path, running_inventory, base_dir,
             base_dir=base_dir,
             about_file_path=about_file_path,
             running_inventory=running_inventory,
-            license_notice_text_location=license_notice_text_location,
+            reference_dir=reference_dir,
         )
         errors.extend(val_err)
     return errors
@@ -972,28 +972,29 @@ def hydrate(self, fields, mapping_file=None):
         return errors
 
     def process(self, fields, about_file_path, running_inventory=False,
-                base_dir=None, license_notice_text_location=None,
+                base_dir=None, reference_dir=None,
                 mapping_file=None):
         """
-        Hydrate and validate a sequence of field name/value tuples from an
-        ABOUT file. Return a list of errors.
+        Validate and set as attributes on this About object a sequence of
+        `fields` name/value tuples. Return a list of errors.
         """
         self.base_dir = base_dir
-        self.license_notice_text_location = license_notice_text_location
+        self.reference_dir = reference_dir
         afp = self.about_file_path
         errors = []
         hydratation_errors = self.hydrate(fields, mapping_file=mapping_file)
         errors.extend(hydratation_errors)
 
         # We want to copy the license_files before the validation
-        if license_notice_text_location:
+        if reference_dir:
             copy_license_notice_files(
-                fields, base_dir, license_notice_text_location, afp)
+                fields, base_dir, reference_dir, afp)
+
         # we validate all fields, not only these hydrated
         all_fields = self.all_fields()
         validation_errors = validate_fields(
             all_fields, about_file_path, running_inventory,
-            self.base_dir, self.license_notice_text_location)
+            self.base_dir, self.reference_dir)
         errors.extend(validation_errors)
 
         # do not forget to resolve about resource paths The
@@ -1043,12 +1044,10 @@ def load(self, location, mapping_file=None):
     # FIXME: an About object should not know about mappings
     def load_dict(self, fields_dict, base_dir, running_inventory=False,
                   mapping_file=None,
-                  license_notice_text_location=None, with_empty=True):
+                  reference_dir=None, with_empty=True):
         """
-        Load the ABOUT file from a fields name/value mapping.
-        If with_empty, create fields with no value for empty fields.
-        Return a list of
-        errors.
+        Load this About object file from a `fields_dict` name/value mapping.
+        Return a list of errors.
         """
         fields = list(fields_dict.items())
         about_file_path = self.about_file_path
@@ -1076,7 +1075,7 @@ def load_dict(self, fields_dict, base_dir, running_inventory=False,
             about_file_path=about_file_path, 
             running_inventory=running_inventory, 
             base_dir=base_dir, 
-            license_notice_text_location=license_notice_text_location,
+            reference_dir=reference_dir,
             mapping_file=mapping_file)
         self.errors = errors
         return errors
@@ -1193,7 +1192,7 @@ def dump_lic(self, location, license_dict):
 # valid field name
 field_name = r'(?P<name>[a-z][0-9a-z_]*)'
 
-valid_field_name = re.compile(field_name, re.UNICODE | re.IGNORECASE).match
+valid_field_name = re.compile(field_name, re.UNICODE | re.IGNORECASE).match  # NOQA
 
 # line in the form of "name: value"
 field_declaration = re.compile(
@@ -1202,7 +1201,7 @@ def dump_lic(self, location, license_dict):
     r'\s*:\s*'
     r'(?P<value>.*)'
     r'\s*$'
-    , re.UNICODE | re.IGNORECASE
+    , re.UNICODE | re.IGNORECASE  # NOQA
     ).match
 
 
@@ -1212,7 +1211,7 @@ def dump_lic(self, location, license_dict):
     r' '
     r'(?P<value>.*)'
     r'\s*$'
-    , re.UNICODE | re.IGNORECASE
+    , re.UNICODE | re.IGNORECASE  # NOQA
     ).match
 
 

src/attributecode/util.py

@@ -598,13 +598,21 @@ def add_unc(location):
 
 
 # FIXME: add docstring
-def copy_license_notice_files(fields, base_dir, license_notice_text_location, afp):
+def copy_license_notice_files(fields, base_dir, reference_dir, afp):
+    """
+    Given a list of (key, value) `fields` tuples and a `base_dir` where ABOUT
+    files and their companion LICENSe are store, and an extra `reference_dir`
+    where reference license an notice files are stored and the `afp`
+    about_file_path value, this function will copy to the base_dir the
+    license_file or notice_file if found in the reference_dir
+    
+    """
     lic_name = ''
     for key, value in fields:
         if key == 'license_file' or key == 'notice_file':
             lic_name = value
 
-            from_lic_path = posixpath.join(to_posix(license_notice_text_location), lic_name)
+            from_lic_path = posixpath.join(to_posix(reference_dir), lic_name)
             about_file_dir = os.path.dirname(to_posix(afp)).lstrip('/')
             to_lic_path = posixpath.join(to_posix(base_dir), about_file_dir)
 

tests/test_gen.py

@@ -177,11 +177,11 @@ def test_generate(self):
     def test_generate_not_overwrite_original_license_file(self):
         location = get_test_loc('test_gen/inv5.csv')
         base_dir = get_temp_dir()
-        license_notice_text_location = None
+        reference_dir = None
         fetch_license = ['url', 'lic_key']
 
         _errors, abouts = gen.generate(
-            location, base_dir, license_notice_text_location, fetch_license)
+            location, base_dir, reference_dir, fetch_license)
 
         in_mem_result = [a.dumps(with_empty=False)for a in abouts][0]
         expected = (

tests/test_model.py

@@ -44,7 +44,7 @@
 from testing_utils import get_test_loc
 
 
-def check_csv(expected, result, regen=True):
+def check_csv(expected, result, regen=False):
     """
     Assert that the contents of two CSV files locations `expected` and
     `result` are equal.

tests/testdata/test_cmd/help/about_gen_help.txt

@@ -8,33 +8,15 @@ Usage: about gen [OPTIONS] LOCATION OUTPUT
   OUTPUT: Path to a directory where ABOUT files are generated.
 
 Options:
-  --fetch-license KEY             Fetch license data and texts from a a DejaCode
-                                  License Library API. Create <license>.LICENSE
-                                  files from the text of each license key side-
-                                  by-side with the generated .ABOUT file. Also
-                                  enhance the .ABOUT file with other data such
-                                  name and category.
-                                  
-                                  The following additional
-                                  options are required:
-                                  
-                                  api_url - URL to the
-                                  DejaCode License Library API endpoint
-                                  
-                                  api_key
-                                  - DejaCode API key
-                                  Example syntax:
-                                  
-                                  about gen
-                                  --fetch-license 'api_url' 'api_key'
-  --license-notice-text-location PATH
-                                  Copy the 'license_file' from the directory to
-                                  the generated location.
-  --mapping                       Use the default file mapping.config
-                                  (./attributecode/mapping.config) with mapping
-                                  between input keys and ABOUT field names.
-  --mapping-file FILE             Use a custom mapping file with mapping between
-                                  input keys and ABOUT field names.
-  -q, --quiet                     Do not print error or warning messages.
-  --verbose                       Show all error and warning messages.
-  -h, --help                      Show this message and exit.
+  --fetch-license URL KEY  Fetch license data and text files from a DejaCode
+                           License Library API URL using the API KEY.
+  --reference DIR          Path to a directory with reference license data and
+                           text files.
+  --mapping                Use the default file mapping.config
+                           (./attributecode/mapping.config) with mapping between
+                           input keys and ABOUT field names.
+  --mapping-file FILE      Use a custom mapping file with mapping between input
+                           keys and ABOUT field names.
+  -q, --quiet              Do not print error or warning messages.
+  --verbose                Show all error and warning messages.
+  -h, --help               Show this message and exit.