Remove filter from inventory, gen and attrib #368

The proper approach is:

 - for gen filter a subset of the input manually
 - for attrib use a custom template for filtering

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

Author: pombredanne

src/attributecode/attrib.py

@@ -31,7 +31,6 @@
 from attributecode.licenses import COMMON_LICENSES
 from attributecode.model import parse_license_expression
 from attributecode.util import add_unc
-from attributecode.util import get_about_file_path
 
 
 # FIXME: the template dir should be outside the code tree
@@ -167,93 +166,26 @@ def generate_from_file(abouts, template_loc=DEFAULT_TEMPLATE_FILE, variables=Non
 
 
 def generate_and_save(abouts, output_location, template_loc=None, variables=None,
-                      mapping_file=None, inventory_location=None):
+                      mapping_file=None):
     """
     Generate an attribution text from an `abouts` list of About objects, a
     `template_loc` template file location and a `variables` optional
     mapping of extra variables. Save the generated attribution text in the
     `output_location` file. 
     Return a list of Error objects if any.
 
-    FIXME: these three argument are too complex:
-
     Optionally use the `mapping_file` mapping config if provided.
-    Optionally filter `abouts` object based on the inventory JSON or CSV at `inventory_location`.
     """
     updated_abouts = []
-    lstrip_afp = []
-    afp_list = []
-    not_match_path = []
     errors = []
 
-    if not inventory_location:
-        updated_abouts = abouts
-
-    # FIXME: this is too complex
-    # Do the following if a filter list (inventory_location) is provided
-    else:
-        if not os.path.exists(inventory_location):
-            # FIXME: this message does not make sense
-            msg = (u'"INVENTORY_LOCATION" does not exist. Generation halted.')
-            errors.append(Error(ERROR, msg))
-            return errors
-
-        if inventory_location.endswith('.csv') or inventory_location.endswith('.json'):
-            # FIXME: we should use the same inventory loading that we use everywhere
-
-            try:
-                # Return a list which contains only the about file path
-                about_list = get_about_file_path(inventory_location, mapping_file=mapping_file)
-            # FIXME: why catching all exceptions?
-            except Exception:
-                # 'about_file_path' key/column doesn't exist
-
-                msg = u"The required key: 'about_file_path' does not exist. Generation halted."
-                errors.append(Error(ERROR, msg))
-                return errors
-        else:
-            # FIXME: this message does not make sense
-            msg = u'Only .csv and .json are supported for the "INVENTORY_LOCATION". Generation halted.'
-            errors.append(Error(ERROR, msg))
-            return errors
-
-        for afp in about_list:
-            lstrip_afp.append(afp.lstrip('/'))
-
-        # return a list of paths that point all to .ABOUT files
-        about_files_list = as_about_paths(lstrip_afp)
-
-        # Collect all the about_file_path
-        for about in abouts:
-            afp_list.append(about.about_file_path)
-
-        # Get the not matching list if any
-        for fp in about_files_list:
-            if not fp in afp_list:
-                not_match_path.append(fp)
-
-        if not_match_path:
-            if len(not_match_path) == len(about_files_list):
-                msg = "None of the paths in the provided 'inventory_location' match with the 'LOCATION'."
-                errors.append(Error(ERROR, msg))
-                return errors
-            else:
-                for path in not_match_path:
-                    msg = 'Path: ' + path + ' cannot be found.'
-                    errors.append(Error(ERROR, msg))
-
-        for about in abouts:
-            for fp in about_files_list:
-                if about.about_file_path == fp:
-                    updated_abouts.append(about)
-
     # Parse license_expression and save to the license list
     for about in updated_abouts:
         if not about.license_expression.value:
             continue
         special_char_in_expression, lic_list = parse_license_expression(about.license_expression.value)
         if special_char_in_expression:
-            msg = (u"The following character(s) cannot be in the licesne_expression: " +
+            msg = (u"The following character(s) cannot be in the license_expression: " +
                    str(special_char_in_expression))
             errors.append(Error(ERROR, msg))
         else:
@@ -274,24 +206,3 @@ def generate_and_save(abouts, output_location, template_loc=None, variables=None
             of.write(rendered)
 
     return errors
-
-
-# FIXME: this function purpose needs to be explained.
-def as_about_paths(paths):
-    """
-    Return a list of paths to .ABOUT files from a list of `paths`
-    strings.
-    """
-    from posixpath import basename
-    from posixpath import dirname
-
-    about_paths = []
-    for path in paths:
-        if path.endswith('.ABOUT'):
-            about_paths.append(path)
-        else:
-            # FIXME: this is not the way to check that a path is a directory, too weak
-            if path.endswith('/'):
-                path += basename(dirname(path))
-            about_paths.append(path + '.ABOUT')
-    return about_paths

src/attributecode/cmd.py

@@ -43,7 +43,6 @@
 from attributecode.model import collect_inventory
 from attributecode.model import write_output
 from attributecode.util import extract_zip
-from attributecode.util import inventory_filter
 
 
 __copyright__ = """
@@ -165,13 +164,6 @@ def validate_extensions(ctx, param, value, extensions=tuple(('.csv', '.json',)))
     metavar='OUTPUT',
     type=click.Path(exists=False, dir_okay=False, writable=True, resolve_path=True))
 
-# fIXME: this is too complex and should be removed
-@click.option('--filter',
-    multiple=True,
-    metavar='<key>=<value>',
-    callback=validate_key_values,
-    help='Filter the inventory to ABOUT matching these key=value e.g. "license_expression=gpl-2.0')
-
 @click.option('-f', '--format',
     is_flag=False,
     default='csv',
@@ -203,7 +195,7 @@ def validate_extensions(ctx, param, value, extensions=tuple(('.csv', '.json',)))
 @click.help_option('-h', '--help')
 
 def inventory(location, output, mapping, mapping_file,
-              format, filter, quiet, verbose):  # NOQA
+              format, quiet, verbose):  # NOQA
     """
 Collect the inventory of .ABOUT file data as CSV or JSON.
 
@@ -224,10 +216,6 @@ def inventory(location, output, mapping, mapping_file,
 
     errors, abouts = collect_inventory(location, mapping_file=mapping_file)
 
-    # FIXME: this is too complex
-    if filter:
-        abouts = inventory_filter(abouts, filter)
-
     # Do not write the output if one of the ABOUT files has duplicated keys
     # TODO: why do this check here?? Also if this is the place, we should list what the errors are.
     dup_error_msg = u'Duplicated keys'
@@ -447,7 +435,6 @@ def attrib(location, output, template, vartext,
         template_loc=template,
         variables=vartext,
         mapping_file=mapping_file,
-        inventory_location=inventory,
     )
     errors.extend(attrib_errors)
 

src/attributecode/util.py

@@ -348,21 +348,6 @@ def format_output(about_data, mapping_file=None):
                 order_dict[other_key] = about_data[other_key]
     return order_dict
 
-# FIXME: why is this used for
-def get_about_file_path(location, mapping_file=None):
-    """
-    Read file at location, return a list of about_file_path.
-    """
-    afp_list = []
-    if location.endswith('.csv'):
-        about_data = load_csv(location, mapping_file=mapping_file)
-    else:
-        about_data = load_json(location)
-
-    for about in about_data:
-        afp_list.append(about['about_file_path'])
-    return afp_list
-
 
 def load_csv(location, mapping_file=None):
     """
@@ -567,34 +552,6 @@ def copy_license_notice_files(fields, base_dir, reference_dir, afp):
                 print('Cannot copy file at %(from_lic_path)r.' % locals())
 
 
-# FIXME: this is NOT a util but something to move with inventories or a method
-# from About objects
-def inventory_filter(abouts, filters):
-    """
-    Return a list of filtered About objects from an `abouts` list of About
-    object using the `filters` mapping of:
-        {field_name: [acceptable_values, ....]}
-
-    ... such that only the About object that have a field_name with a value that
-    matches one of the acceptable values is returned. Other About object are
-    filtered out.
-    """
-    matching_abouts = []
-    for about in abouts:
-        for field_name, acceptable_values in filters.items():
-            # Check if the about object has the filtered attribute and if the
-            # attributed value is the same as the defined in the filter
-            actual_value = getattr(about, field_name, None)
-            if actual_value in acceptable_values and not about in matching_abouts:
-                matching_abouts.append(about)
-                # FIXME: if it matches once it matches always which is probably not right
-                break
-
-    return matching_abouts
-
-
-
-
 # FIXME: we should use a license object instead
 def ungroup_licenses(licenses):
     """

tests/test_util.py

@@ -397,13 +397,6 @@ def test_load_csv_with_mapping(self):
         result = util.load_csv(test_file, mapping_file=DEFAULT_MAPPING)
         assert expected == result
 
-    def test_get_about_file_path_from_csv_using_mapping(self):
-        test_file = get_test_loc('test_util/csv/about.csv')
-        expected = ['about.ABOUT']
-        result = util.get_about_file_path(
-            test_file, mapping_file=DEFAULT_MAPPING)
-        assert expected == result
-
     def test_load_csv_does_convert_column_names_to_lowercase(self):
         test_file = get_test_loc('test_util/csv/about_key_with_upper_case.csv')
         expected = [OrderedDict(
@@ -474,13 +467,6 @@ def test_load_non_list_json(self):
         result = util.load_json(test_file)
         assert expected == result
 
-    # FIXME: mappings are a CSV-only feature!!!!!
-    def test_get_about_file_path_from_json_using_mapping(self):
-        test_file = get_test_loc('test_util/json/expected.json')
-        expected = ['/load/this.ABOUT']
-        result = util.get_about_file_path(test_file, mapping_file=DEFAULT_MAPPING)
-        assert expected == result
-
     def test_load_non_list_json2(self):
         test_file = get_test_loc('test_util/json/not_a_list.json')
         expected = [OrderedDict([
@@ -632,17 +618,6 @@ def test_load_yaml_about_file_with_multiline(self):
             # notes: exceptio is rasied only for the first dupe
             assert 'Duplicate key in YAML source: owner' == str(e)
 
-    def test_inventory_filter(self):
-        test_loc = get_test_loc('test_util/inventory_filter')
-        _errors, abouts = model.collect_inventory(test_loc)
-
-        filter_dict = {'name': ['simple']}
-        # The test loc has 2 .about files, only the simple.about is taken after
-        # the filtering
-        updated_abouts = util.inventory_filter(abouts, filter_dict)
-        for about in updated_abouts:
-            assert about.name.value == 'simple'
-
     def test_ungroup_licenses(self):
         about = [
             OrderedDict([

tests/testdata/test_cmd/help/about_inventory_help.txt

@@ -7,8 +7,6 @@ Usage: about inventory [OPTIONS] LOCATION OUTPUT
   OUTPUT: Path to the JSON or CSV inventory file to create.
 
 Options:
-  --filter <key>=<value>   Filter the inventory to ABOUT matching these
-                           key=value e.g. "license_expression=gpl-2.0
   -f, --format [json|csv]  Set OUTPUT inventory file format.  [default: csv]
   --mapping                Use the default built-in "mapping.config" file with
                            mapping between input keys and .ABOUT field