Repository: SpiNNakerManchester/SpiNNUtils

Issue:
Broken citation code
The citation code does not handle comments in `requirements.txt`. The format supports comments (with `#`), therefore not handling it is ***wrong!***

Discussion:

Patch:
diff --git a/spinn_utilities/citation/citation_aggregator.py b/spinn_utilities/citation/citation_aggregator.py
index 295dffa..0c37124 100644
--- a/spinn_utilities/citation/citation_aggregator.py
+++ b/spinn_utilities/citation/citation_aggregator.py
@@ -7,9 +7,9 @@ import argparse
 import sys
 
 from spinn_utilities.citation import CitationUpdaterAndDoiGenerator
-import traceback
 
 REQUIREMENTS_FILE = "requirements.txt"
+C_REQUIREMENTS_FILE = "c_requirements.txt"
 CITATION_FILE = "CITATION.cff"
 PYPI_TO_IMPORT_FILE = "pypi_to_import"
 
@@ -51,7 +51,8 @@ class CitationAggregator(object):
         # get the top citation file to add references to
         top_citation_file_path = os.path.join(os.path.dirname(os.path.dirname(
             os.path.abspath(module_to_start_at.__file__))), CITATION_FILE)
-        modules_seen_so_far = list()
+        modules_seen_so_far = set()
+        modules_seen_so_far.add("")  # Make sure the empty entry is absent
         with open(top_citation_file_path, 'r') as stream:
             top_citation_file = yaml.safe_load(stream)
         top_citation_file[REFERENCES_YAML_POINTER] = list()
@@ -59,8 +60,10 @@ class CitationAggregator(object):
         # get the dependency list
         requirements_file_path = os.path.join(
             os.path.dirname(os.path.dirname(os.path.abspath(
-                module_to_start_at.__file__))),
-            REQUIREMENTS_FILE)
+                module_to_start_at.__file__))), REQUIREMENTS_FILE)
+        c_requirements_file_path = os.path.join(
+            os.path.dirname(os.path.dirname(os.path.abspath(
+                module_to_start_at.__file__))), C_REQUIREMENTS_FILE)
 
         # attempt to get python PYPI to import command map
         pypi_to_import_map_file = os.path.join(
@@ -73,14 +76,33 @@ class CitationAggregator(object):
                 pypi_to_import_map_file)
 
         if os.path.isfile(requirements_file_path):
-            for line in open(requirements_file_path, "r"):
-                module_to_get_requirements_for = line.split(" ")[0]
-                module_to_get_requirements_for = \
-                    module_to_get_requirements_for.split("\n")[0]
-                if module_to_get_requirements_for not in modules_seen_so_far:
-                    self._handle_dependency(
-                        top_citation_file, module_to_get_requirements_for,
-                        pypi_to_import_map, modules_seen_so_far)
+            with open(requirements_file_path, "r") as r_file:
+                for line in r_file:
+                    module = line.strip()
+                    if module.startswith("#"):
+                        continue
+                    if module not in modules_seen_so_far:
+                        import_name = pypi_to_import_map.get(module, module)
+                        try:
+                            imported_module = importlib.import_module(
+                                import_name)
+                            self._handle_python_dependency(
+                                top_citation_file, imported_module,
+                                modules_seen_so_far,
+                                pypi_to_import_map[module])
+                        except Exception as e:  # pragma: no cover
+                            print("Error handling python dependency {}: {}"
+                                  .format(module, str(e)))
+
+        if os.path.isfile(c_requirements_file_path):
+            with open(c_requirements_file_path, "r") as r_file:
+                for line in r_file:
+                    module = line.strip()
+                    if module.startswith("#"):
+                        continue
+                    if module not in modules_seen_so_far:
+                        self._handle_c_dependency(
+                            top_citation_file, module, modules_seen_so_far)
 
         # write citation file with updated fields
         with io.open(
@@ -101,64 +123,21 @@ class CitationAggregator(object):
             pypi_to_import_map[pypi] = import_command.split("\n")[0]
         return pypi_to_import_map
 
-    # noinspection PyBroadException
-    def _handle_dependency(
-            self, top_citation_file, module_to_get_requirements_for,
-            pypi_to_import_map, modules_seen_so_far):
-        """ Handle a dependency, assumes its either python or c code
-
-        :param top_citation_file: YAML file for the top citation file
-        :param module_to_get_requirements_for: module to import
-        :type top_citation_file: YAML file
-        :type module_to_get_requirements_for: str
-        :param pypi_to_import_map: map between PYPI name and the python import
-        :type pypi_to_import_map: dict
-        :param modules_seen_so_far:
-        :type modules_seen_so_far:
-        :return: None
-        """
-
-        # determine name for import
-        if module_to_get_requirements_for in pypi_to_import_map:
-            import_name = pypi_to_import_map[module_to_get_requirements_for]
-        else:
-            import_name = module_to_get_requirements_for
-        try:
-            imported_module = importlib.import_module(import_name)
-            self._handle_python_dependency(
-                top_citation_file, imported_module, modules_seen_so_far,
-                pypi_to_import_map[module_to_get_requirements_for])
-        except ImportError:
-            # assume now that its a c code module.
-            self._handle_c_dependency(
-                top_citation_file, module_to_get_requirements_for,
-                modules_seen_so_far)
-        except Exception:  # pragma: no cover
-            print("Error handling dependency {}".format(
-                module_to_get_requirements_for))
-            traceback.print_exc()
-
     def _handle_c_dependency(
-            self, top_citation_file, module_to_get_requirements_for,
-            modules_seen_so_far):
+            self, top_citation_file, module, modules_seen_so_far):
         """ Handle a c code dependency
 
         :param top_citation_file: YAML file for the top citation file
-        :param module_to_get_requirements_for: module to import
+        :param module: module to find
         :type top_citation_file: YAML file
-        :type module_to_get_requirements_for: str
+        :type module: str
         :return: None
         """
-        if not module_to_get_requirements_for.startswith("#"):
-            print("Unknown dependency {}".format(
-                module_to_get_requirements_for))
-            return
-        true_software_name = module_to_get_requirements_for.split("#")[1]
-        cleaned_path = self.locate_path_for_c_dependency(true_software_name)
+        cleaned_path = self.locate_path_for_c_dependency(module)
         if cleaned_path is not None:
             # process reference
             reference_entry = self._process_reference(
-                cleaned_path, None, modules_seen_so_far, true_software_name)
+                cleaned_path, None, modules_seen_so_far, module)
 
             # append to the top citation file
             top_citation_file[REFERENCES_YAML_POINTER].append(
@@ -166,8 +145,7 @@ class CitationAggregator(object):
             self._search_for_other_c_references(
                 reference_entry, cleaned_path, modules_seen_so_far)
         else:
-            print("Could not find C dependency {}".format(
-                module_to_get_requirements_for))
+            print("Could not find C dependency {}".format(module))
 
     @staticmethod
     def locate_path_for_c_dependency(true_software_name):
@@ -207,7 +185,7 @@ class CitationAggregator(object):
                 reference_entry[REFERENCES_YAML_POINTER] = list()
                 reference_entry[REFERENCES_YAML_POINTER].append(
                     dependency_reference_entry)
-                modules_seen_so_far.append(
+                modules_seen_so_far.add(
                     possible_extra_citation_file.split(".")[0])
 
     def _handle_python_dependency(
@@ -283,7 +261,7 @@ class CitationAggregator(object):
             reference_entry = self._try_to_find_version(
                 imported_module, module_name)
 
-        modules_seen_so_far.append(imported_module)
+        modules_seen_so_far.add(imported_module)
         return reference_entry
 
     @staticmethod


Test patch:
diff --git a/unittests/citation/package_1_folder/c_requirements.txt b/unittests/citation/package_1_folder/c_requirements.txt
new file mode 100644
index 0000000..58eed49
--- /dev/null
+++ b/unittests/citation/package_1_folder/c_requirements.txt
@@ -0,0 +1,3 @@
+# A list of C dependencies
+c_module
+nonexiststent_c
diff --git a/unittests/citation/package_1_folder/requirements.txt b/unittests/citation/package_1_folder/requirements.txt
index eb15ee9..3480223 100644
--- a/unittests/citation/package_1_folder/requirements.txt
+++ b/unittests/citation/package_1_folder/requirements.txt
@@ -1,8 +1,7 @@
+# A list of dependencies
 package_2
 package_3
 package_4
 package_5
 pyyaml
 nonexistent
-#c_module
-#nonexiststent_c
diff --git a/unittests/citation/test_citation.py b/unittests/citation/test_citation.py
index e5a915f..421a1ec 100644
--- a/unittests/citation/test_citation.py
+++ b/unittests/citation/test_citation.py
@@ -36,6 +36,7 @@ def test_generate_aggregate():
                  "--publish_doi"]
     httpretty.enable()
     generate_aggregate(arguments)
+    httpretty.disable()
     assert os.path.isfile(output_path)
     with open(output_path, 'r') as stream:
         aggregate = yaml.safe_load(stream)