asclepias/asclepias-broker

Suggestions for refactoring

Opened this issue · 0 comments

diff --git a/INSTALL.rst b/INSTALL.rst
index 151ff54..212bfc0 100644
--- a/INSTALL.rst
+++ b/INSTALL.rst
@@ -23,6 +23,7 @@ Then, create and run the services using the docker containers:
 .. code-block:: console
 
     $ cd asclepias-broker
+# This should just be scripts/bootstrap + scripts/setup (using just docker-compose up)
     $ docker-compose up db es kibana
 
 Database and search index
diff --git a/README.rst b/README.rst
index 7c2808c..7a565a5 100644
--- a/README.rst
+++ b/README.rst
@@ -25,3 +25,4 @@ https://asclepias-broker.readthedocs.io/
 
 The code in this repository was funded by a grant from the Alfred P. Sloan
 Foundation to the American Astronomical Society (2016).
+# Needs content - start with default from cookiecutter template.
diff --git a/asclepias_broker/api/ingestion.py b/asclepias_broker/api/ingestion.py
index 76db804..61daaa4 100644
--- a/asclepias_broker/api/ingestion.py
+++ b/asclepias_broker/api/ingestion.py
@@ -17,7 +17,7 @@ from ..models import Group, GroupM2M, GroupMetadata, GroupRelationship, \
     GroupRelationshipM2M, GroupRelationshipMetadata, GroupType, Identifier, \
     Identifier2Group, Relation, Relationship, Relationship2GroupRelationship
 
-
+# Decompose function in to smaller parts
 def merge_group_relationships(group_a, group_b, merged_group):
     """Merge the relationships of merged groups A and B to avoid collisions.
 
@@ -171,6 +171,8 @@ def delete_duplicate_relationship_m2m(group_a, group_b,
 
     for queried_fk, grouping_fk in [(queried_fk, grouping_fk),
                                     (grouping_fk, queried_fk), ]:
+        # E.g.: candidate for separate function? Looks identical to function
+        # below
         left_gr = aliased(cls, name='left_gr')
         right_gr = aliased(cls, name='right_gr')
         left_queried_fk = getattr(left_gr, queried_fk)
diff --git a/asclepias_broker/mappings/fixtures.py b/asclepias_broker/mappings/fixtures.py
index c60095b..745adc5 100644
--- a/asclepias_broker/mappings/fixtures.py
+++ b/asclepias_broker/mappings/fixtures.py
@@ -17,6 +17,7 @@ from faker import Faker
 from ..api import RelationshipAPI
 from .dsl import ObjectDoc, ObjectRelationshipsDoc
 
+# Move all test data / fixture data creation to a separate package?
 
 #
 # Utils
diff --git a/asclepias_broker/views.py b/asclepias_broker/views.py
index 8cd0c34..f571292 100644
--- a/asclepias_broker/views.py
+++ b/asclepias_broker/views.py
@@ -26,6 +26,9 @@ blueprint = Blueprint('asclepias_ui', __name__, template_folder='templates')
 #
 # UI Views
 #
+
+# Either 1) create "views" package with "ui.py" and "api.py" each with one
+# blueprint or 2) create two different module packages with each a "views.py"
 @blueprint.route('/list')
 def listpids():
     """Renders all identifiers in the system."""
@@ -71,7 +74,7 @@ def relationships():
 #
 api_blueprint = Blueprint('asclepias_api', __name__)
 
-
+# Move to errors.py?
 class ObjectNotFoundRESTError(RESTException):
     """Object not found error."""
 
@@ -83,7 +86,7 @@ class ObjectNotFoundRESTError(RESTException):
         self.description = \
             'No object found with identifier [{}]'.format(identifier)
 
-
+# Move to errors.py?
 class PayloadValidationRESTError(RESTException):
     """Invalid payload error."""
 
diff --git a/requirements-devel.txt b/requirements-devel.txt
index 7d773e8..0395023 100644
--- a/requirements-devel.txt
+++ b/requirements-devel.txt
@@ -4,3 +4,5 @@
 #
 # Asclepias Broker is free software; you can redistribute it and/or modify it
 # under the terms of the MIT License; see LICENSE file for more details.
+
+# This file can be removed
diff --git a/setup.py b/setup.py
index 5137493..caa2af0 100644
--- a/setup.py
+++ b/setup.py
@@ -24,6 +24,7 @@ tests_require = [
     'mock>=2.0.0',
     'pydocstyle>=2.0.0',
     'pytest>=3.3.1',
+    # pytest-cache can be removed
     'pytest-cache>=1.0',
     'pytest-cov>=2.5.1',
     'pytest-flask>=0.10.0',
@@ -50,11 +51,13 @@ setup_requires = [
 
 install_requires = [
     'arrow>=0.12.1',
+    # Test requirement or real requirement?
     'faker>=0.8.13',
     'Flask-Debugtoolbar>=0.10.1',
     'idutils>=1.0.0',
     'invenio[base,auth,{db},{es}]==3.0.0rc1'.format(
         db=DATABASE, es=ELASTICSEARCH),
+    # Not part of Invenio-JSONSchemas?
     'jsonschema>=2.6.0',
     'marshmallow>=2.15.0',
     'webargs>=2.1.0',
diff --git a/tests/helpers.py b/tests/helpers.py
index 207ac9c..e5845c6 100644
--- a/tests/helpers.py
+++ b/tests/helpers.py
@@ -183,7 +183,7 @@ def generate_payloads(input_items, event_schema=None):
         jsonschema.validate(events, {'type': 'array', 'items': event_schema})
     return events
 
-
+# Is it possible to decompose the two functions below into smaller parts?
 def create_objects_from_relations(relationships: List[Tuple],
                                   metadata: List[Tuple[dict]]=None):
     """Given a list of relationships, create all corresponding DB objects.