Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Suggestions for refactoring #36

Open
lnielsen opened this issue Apr 18, 2018 · 0 comments
Open

Suggestions for refactoring #36

lnielsen opened this issue Apr 18, 2018 · 0 comments

Comments

@lnielsen
Copy link
Collaborator

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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant