Repository: wearewhys/magnivore

Issue:
Improve rulesets naming for join section
The join section should have better keyword naming:

- *joins* should be renamed to *sources*

Discussion:

Patch:
diff --git a/README.rst b/README.rst
index 5ec15ba..8dca6f7 100644
--- a/README.rst
+++ b/README.rst
@@ -12,7 +12,7 @@ A simple migration rule::
 
     {
         "profiles": {
-            "joins": [
+            "sources": [
                 {"table": "users"},
                 {"table": "addresses", "on":"user"}
             ],
diff --git a/magnivore/RulesParser.py b/magnivore/RulesParser.py
index 85def1c..47a81af 100644
--- a/magnivore/RulesParser.py
+++ b/magnivore/RulesParser.py
@@ -21,7 +21,7 @@ class RulesParser():
 
     def _process(self, table, table_rules):
         model = self.receiver[table]
-        targets = self.targets.get(table_rules['joins'])
+        targets = self.targets.get(table_rules['sources'])
 
         if 'transform' in table_rules:
             transformations = table_rules['transform']
diff --git a/magnivore/Targets.py b/magnivore/Targets.py
index 27ce660..31a311e 100644
--- a/magnivore/Targets.py
+++ b/magnivore/Targets.py
@@ -8,23 +8,23 @@ class Targets:
         self.source_models = source_models
         self.logger = logger
 
-    def _apply_aggregation(self, query, joins):
-        model = self.source_models[joins['table']]
-        model_field = getattr(model,  joins['aggregation']['group'])
+    def _apply_aggregation(self, query, source):
+        model = self.source_models[source['table']]
+        model_field = getattr(model,  source['aggregation']['group'])
         query = query.group_by(model_field)
-        aggregation = joins['aggregation']['function']
+        aggregation = source['aggregation']['function']
         if aggregation == 'count':
             aggregation_function = fn.Count(model_field)
-        condition = joins['aggregation']['condition']
+        condition = source['aggregation']['condition']
         if condition['operator'] == 'gt':
             query = query.having(aggregation_function > condition['value'])
         elif condition['operator'] == 'eq':
             query = query.having(aggregation_function == condition['value'])
         return query
 
-    def _apply_condition(self, query, joins):
-        conditions = joins['conditions']
-        model = self.source_models[joins['table']]
+    def _apply_condition(self, query, source):
+        conditions = source['conditions']
+        model = self.source_models[source['table']]
         for field, condition in conditions.items():
             model_field = getattr(model, field)
             if type(condition) == dict:
@@ -43,15 +43,15 @@ class Targets:
                 query = query.where(model_field == condition)
         return query
 
-    def _apply_join(self, query, join, models):
-        model = self.source_models[join['table']]
+    def _apply_join(self, query, source, models):
+        model = self.source_models[source['table']]
 
         previous_model = models[models.index(model)-1]
-        if 'switch' in join:
-            if join['switch']:
+        if 'switch' in source:
+            if source['switch']:
                 previous_model = models[0]
 
-        on = join['on']
+        on = source['on']
         if type(on) is list:
             left_side = getattr(previous_model, on[0])
             right_side = getattr(model, on[1])
@@ -59,44 +59,44 @@ class Targets:
         else:
             expression = getattr(model, on)
 
-        if 'switch' in join:
+        if 'switch' in source:
             query = query.switch(models[0])
 
-        if 'outer' in join:
+        if 'outer' in source:
             return query.join(model, 'LEFT OUTER', on=expression)
         return query.join(model, on=expression)
 
-    def _apply_pick(self, query, join):
-        model = self.source_models[join['table']]
+    def _apply_pick(self, query, source):
+        model = self.source_models[source['table']]
         selects = []
-        for column, value in join['picks'].items():
+        for column, value in source['picks'].items():
             if value is True:
                 selects.append(getattr(model, column))
             elif value == 'sum':
                 selects.append(fn.Sum(getattr(model, column)))
         return query.select(*selects)
 
-    def get(self, joins, limit=None, offset=0):
+    def get(self, sources, limit=None, offset=0):
         """
         Retrieves the targets for the given joins
         """
-        if len(joins) == 0:
+        if len(sources) == 0:
             raise ValueError
 
         aggregations = []
         conditions = []
         models = []
         picks = []
-        for join in joins:
-            models.append(self.source_models[join['table']])
-            if 'conditions' in join:
-                conditions.append(join)
+        for source in sources:
+            models.append(self.source_models[source['table']])
+            if 'conditions' in source:
+                conditions.append(source)
 
-            if 'aggregation' in join:
-                aggregations.append(join)
+            if 'aggregation' in source:
+                aggregations.append(source)
 
-            if 'picks' in join:
-                picks.append(join)
+            if 'picks' in source:
+                picks.append(source)
 
         query = models[0]
         if picks == []:
@@ -104,9 +104,9 @@ class Targets:
         for pick in picks:
             query = self._apply_pick(query, pick)
 
-        joins.pop(0)
-        for join in joins:
-            query = self._apply_join(query, join, models)
+        sources.pop(0)
+        for source in sources:
+            query = self._apply_join(query, source, models)
 
         for condition in conditions:
             query = self._apply_condition(query, condition)


Test patch:
diff --git a/tests/integration/RulesParser.py b/tests/integration/RulesParser.py
index 76ca84a..c5ba559 100644
--- a/tests/integration/RulesParser.py
+++ b/tests/integration/RulesParser.py
@@ -9,7 +9,7 @@ from pytest import fixture
 def rules():
     rules = {
         'profiles': {
-            'joins': [
+            'sources': [
                 {'table': 'users'},
                 {'table': 'addresses', 'on': 'user'}
             ],
@@ -60,7 +60,7 @@ def test_transform_match(logger, config_setup, donor_setup, receiver_setup,
     rules['profiles']['track'] = 'editor'
     article_rules = {
         'articles': {
-            'joins': [
+            'sources': [
                 {'table': 'posts'}
             ],
             'transform': {
diff --git a/tests/integration/Targets.py b/tests/integration/Targets.py
index 1c2aa63..3422798 100644
--- a/tests/integration/Targets.py
+++ b/tests/integration/Targets.py
@@ -14,11 +14,11 @@ def interface(config_setup):
 
 def test_get(interface, donor_setup, receiver_setup, tracker_setup):
     targets = Targets(interface.donor(), Logger())
-    joins = [
+    sources = [
         {'table': 'users'},
         {'table': 'addresses', 'on': 'user'}
     ]
-    items = targets.get(joins)
+    items = targets.get(sources)
     results = []
     for item in items:
         results.append(item)
diff --git a/tests/unit/RulesParser.py b/tests/unit/RulesParser.py
index 911f2da..c25f1e0 100644
--- a/tests/unit/RulesParser.py
+++ b/tests/unit/RulesParser.py
@@ -14,7 +14,7 @@ from pytest import fixture
 def rules():
     rules = {
         'profiles': {
-            'joins': [
+            'sources': [
                 {'table': 'nodes'},
                 {'table': 'points', 'on': 'node'}
             ],
diff --git a/tests/unit/Targets.py b/tests/unit/Targets.py
index 159af9c..70d3703 100644
--- a/tests/unit/Targets.py
+++ b/tests/unit/Targets.py
@@ -31,12 +31,12 @@ def targets(nodes, points, logger):
 
 
 @fixture
-def joins():
-    joins = [
+def sources():
+    sources = [
         {'table': 'nodes'},
         {'table': 'points', 'on': 'node'}
     ]
-    return joins
+    return sources
 
 
 @fixture(params=['gt', 'lt', 'not'])
@@ -56,113 +56,113 @@ def test_get_targets_empty(targets):
         targets.get([])
 
 
-def test_get(targets, joins, nodes, nodes_query, points):
-    targets.get(joins)
+def test_get(targets, sources, nodes, nodes_query, points):
+    targets.get(sources)
     nodes.select.assert_called_with(nodes, points)
     nodes_query.join.assert_called_with(points, on=points.node)
     assert nodes_query.join().execute.call_count == 1
 
 
-def test_get_triple_join(targets, joins, nodes, nodes_query, points):
+def test_get_triple_join(targets, sources, nodes, nodes_query, points):
     dots = MagicMock()
     targets.source_models['dots'] = dots
-    joins.append({'table': 'dots', 'on': ['id', 'point']})
-    targets.get(joins)
+    sources.append({'table': 'dots', 'on': ['id', 'point']})
+    targets.get(sources)
     nodes_query.join().join.assert_called_with(dots, on=False)
     assert nodes_query.join().join().execute.call_count == 1
 
 
-def test_get_limit(targets, joins, nodes, nodes_query, points):
-    targets.get(joins, limit=100)
+def test_get_limit(targets, sources, nodes, nodes_query, points):
+    targets.get(sources, limit=100)
     nodes_query.join().limit.assert_called_with(100)
     assert nodes_query.join().limit().offset().execute.call_count == 1
 
 
-def test_get_limit_with_offset(targets, joins, nodes, nodes_query, points):
-    targets.get(joins, limit=100, offset=10)
+def test_get_limit_with_offset(targets, sources, nodes, nodes_query, points):
+    targets.get(sources, limit=100, offset=10)
     nodes_query.join().limit().offset.assert_called_with(10)
     assert nodes_query.join().limit().offset().execute.call_count == 1
 
 
-def test_get_switch(targets, joins, nodes, nodes_query, points):
-    joins[1]['switch'] = True
-    targets.get(joins)
+def test_get_switch(targets, sources, nodes, nodes_query, points):
+    sources[1]['switch'] = True
+    targets.get(sources)
     nodes.select.assert_called_with(nodes, points)
     nodes_query.switch().join.assert_called_with(points, on=points.node)
     assert nodes_query.switch().join().execute.call_count == 1
 
 
-def test_get_join_on(targets, joins, nodes, nodes_query, points):
-    joins[1]['on'] = ['id', 'node']
-    targets.get(joins)
+def test_get_join_on(targets, sources, nodes, nodes_query, points):
+    sources[1]['on'] = ['id', 'node']
+    targets.get(sources)
     nodes.select.assert_called_with(nodes, points)
     nodes_query.join.assert_called_with(points, on=(nodes.id == points.node))
     assert nodes_query.join().execute.call_count == 1
 
 
-def test_get_join_outer(targets, joins, nodes, nodes_query, points):
-    joins[1]['outer'] = True
-    targets.get(joins)
+def test_get_join_outer(targets, sources, nodes, nodes_query, points):
+    sources[1]['outer'] = True
+    targets.get(sources)
     nodes.select.assert_called_with(nodes, points)
     nodes_query.join.assert_called_with(points, 'LEFT OUTER', on=points.node)
     assert nodes_query.join().execute.call_count == 1
 
 
-def test_get_conditions(targets, joins, nodes, nodes_query):
-    joins[0]['conditions'] = {
+def test_get_conditions(targets, sources, nodes, nodes_query):
+    sources[0]['conditions'] = {
         'somefield': 'myvalue'
     }
-    joins[1]['conditions'] = {
+    sources[1]['conditions'] = {
         'somefield': 'myvalue'
     }
-    targets.get(joins)
+    targets.get(sources)
     expression = (nodes.somefield == 'myvalue')
     nodes_query.join().where().where.assert_called_with(expression)
     assert nodes_query.join().where().where().execute.call_count == 1
 
 
-def test_get_conditions_greater(operator, targets, joins, nodes, nodes_query):
-    joins[0]['conditions'] = {
+def test_get_conditions_greater(operator, targets, sources, nodes):
+    sources[0]['conditions'] = {
         'somefield': {
             'operator': operator[0],
             'value': 'myvalue'
         }
     }
-    targets.get(joins)
-    nodes_query.join().where.assert_called_with(operator[1])
-    assert nodes_query.join().where().execute.call_count == 1
+    targets.get(sources)
+    nodes.select().join().where.assert_called_with(operator[1])
+    assert nodes.select().join().where().execute.call_count == 1
 
 
-def test_get_conditions_in(targets, joins, nodes, nodes_query):
-    joins[0]['conditions'] = {
+def test_get_conditions_in(targets, sources, nodes, nodes_query):
+    sources[0]['conditions'] = {
         'somefield': {
             'operator': 'in',
             'value': ['myvalue']
         }
     }
-    targets.get(joins)
+    targets.get(sources)
     expression = (nodes.somefield << ['myvalue'])
     nodes_query.join().where.assert_called_with(expression)
     assert nodes_query.join().where().execute.call_count == 1
 
 
-def test_get_conditions_isnull(targets, joins, nodes, nodes_query):
-    joins[0]['conditions'] = {
+def test_get_conditions_isnull(targets, sources, nodes, nodes_query):
+    sources[0]['conditions'] = {
         'somefield': {
             'operator': 'isnull',
             'value': True
         }
     }
-    targets.get(joins)
+    targets.get(sources)
     expression = (nodes.somefield.is_null(True))
     nodes_query.join().where.assert_called_with(expression)
     assert nodes_query.join().where().execute.call_count == 1
 
 
-def test_get_aggregations(mocker, targets, joins, nodes, nodes_query):
+def test_get_aggregations(mocker, targets, sources, nodes, nodes_query):
     mocker.patch.object(fn, 'Count')
     fn.Count.return_value = 0
-    joins[0]['aggregation'] = {
+    sources[0]['aggregation'] = {
         'function': 'count',
         'group': 'email',
         'condition': {
@@ -170,16 +170,16 @@ def test_get_aggregations(mocker, targets, joins, nodes, nodes_query):
             'value': 1
         }
     }
-    targets.get(joins)
+    targets.get(sources)
     nodes_query.join().group_by.assert_called_with(nodes.email)
     nodes_query.join().group_by().having.assert_called_with(fn.Count() > 1)
     assert nodes_query.join().group_by().having().execute.call_count == 1
 
 
-def test_get_aggregations_eq(mocker, targets, joins, nodes, nodes_query):
+def test_get_aggregations_eq(mocker, targets, sources, nodes, nodes_query):
     mocker.patch.object(fn, 'Count')
     fn.Count.return_value = 0
-    joins[0]['aggregation'] = {
+    sources[0]['aggregation'] = {
         'function': 'count',
         'group': 'email',
         'condition': {
@@ -187,37 +187,37 @@ def test_get_aggregations_eq(mocker, targets, joins, nodes, nodes_query):
             'value': 1
         }
     }
-    targets.get(joins)
+    targets.get(sources)
     nodes_query.join().group_by.assert_called_with(nodes.email)
     nodes_query.join().group_by().having.assert_called_with(fn.Count() == 1)
     assert nodes_query.join().group_by().having().execute.call_count == 1
 
 
-def test_get_picks(targets, joins, nodes, nodes_query):
-    joins[0]['picks'] = {
+def test_get_picks(targets, sources, nodes, nodes_query):
+    sources[0]['picks'] = {
         'field': True
     }
-    targets.get(joins)
+    targets.get(sources)
     nodes.select.assert_called_with(nodes.field)
     assert nodes_query.join().execute.call_count == 1
 
 
-def test_get_picks_sum(targets, joins, nodes, nodes_query):
-    joins[0]['picks'] = {
+def test_get_picks_sum(targets, sources, nodes, nodes_query):
+    sources[0]['picks'] = {
         'field': 'sum'
     }
-    targets.get(joins)
+    targets.get(sources)
     nodes.select.assert_called_with(fn.Sum(nodes.field))
     assert nodes_query.join().execute.call_count == 1
 
 
-def test_get_log_query(targets, joins, nodes_query, logger):
-    targets.get(joins)
+def test_get_log_query(targets, sources, nodes_query, logger):
+    targets.get(sources)
     calls = [call.logger.log('get-targets', nodes_query.join())]
     logger.log.assert_has_calls(calls)
 
 
-def test_get_log_targets_count(targets, joins, nodes_query, logger):
-    targets.get(joins)
+def test_get_log_targets_count(targets, sources, nodes_query, logger):
+    targets.get(sources)
     calls = [call.logger.log('get-targets-count', nodes_query.join().count())]
     logger.log.assert_has_calls(calls)