Repository: opensistemas-hub/osbrain
Pull request #140 (2017-06-16 10:52:37)

Refactor agent.py 
When I was looking as to why the subscription from osmarkets was failing, I discovered two pieces of code that looked very similar and extracted them into `_topics_to_bytes`.

Also, changed `subscribe` to `_subscribe`.

I will squash the commits if approved.

Peque: ![](http://i.imgur.com/xsMxqpy.png)

:thinking:
codecov[bot]: # [Codecov](https://codecov.io/gh/opensistemas-hub/osbrain/pull/140?src=pr&el=h1) Report
> Merging [#140](https://codecov.io/gh/opensistemas-hub/osbrain/pull/140?src=pr&el=desc) into [master](https://codecov.io/gh/opensistemas-hub/osbrain/commit/372e7d62b9cf4364f89cf2573d340a843a43a86f?src=pr&el=desc) will **decrease** coverage by `0.28%`.
> The diff coverage is `100%`.

[![Impacted file tree graph](https://codecov.io/gh/opensistemas-hub/osbrain/pull/140/graphs/tree.svg?src=pr&token=LfvMoNOrOc&width=650&height=150)](https://codecov.io/gh/opensistemas-hub/osbrain/pull/140?src=pr&el=tree)

```diff
@@            Coverage Diff             @@
##           master     #140      +/-   ##
==========================================
- Coverage   98.16%   97.88%   -0.29%     
==========================================
  Files          25       25              
  Lines        3114     3114              
  Branches      243      241       -2     
==========================================
- Hits         3057     3048       -9     
- Misses         42       46       +4     
- Partials       15       20       +5
```


| [Impacted Files](https://codecov.io/gh/opensistemas-hub/osbrain/pull/140?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [osbrain/agent.py](https://codecov.io/gh/opensistemas-hub/osbrain/pull/140?src=pr&el=tree#diff-b3NicmFpbi9hZ2VudC5weQ==) | `95.9% <100%> (ø)` | :arrow_up: |
| [osbrain/tests/test\_agent\_serialization.py](https://codecov.io/gh/opensistemas-hub/osbrain/pull/140?src=pr&el=tree#diff-b3NicmFpbi90ZXN0cy90ZXN0X2FnZW50X3NlcmlhbGl6YXRpb24ucHk=) | `96.84% <0%> (-3.16%)` | :arrow_down: |
| [osbrain/tests/test\_agent.py](https://codecov.io/gh/opensistemas-hub/osbrain/pull/140?src=pr&el=tree#diff-b3NicmFpbi90ZXN0cy90ZXN0X2FnZW50LnB5) | `97.8% <0%> (-0.95%)` | :arrow_down: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/opensistemas-hub/osbrain/pull/140?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/opensistemas-hub/osbrain/pull/140?src=pr&el=footer). Last update [372e7d6...e4469d8](https://codecov.io/gh/opensistemas-hub/osbrain/pull/140?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

Flood1993: Extracted method into a function, and moved it to common.py.
Flood1993: Added tests and slightly modified the function so as to check passed topics are valid.
Flood1993: Removed manual checks as suggested. Also updated counters with errors.
Flood1993: Incorporated lastest suggestions. However, for the parametrization, I used `[b'', b'U']` instead of `[None, b'U']`, since the former is the default value of the keyword argument.

Patch:
diff --git a/osbrain/agent.py b/osbrain/agent.py
index 9bba6e5..602b9ab 100644
--- a/osbrain/agent.py
+++ b/osbrain/agent.py
@@ -24,6 +24,7 @@ import zmq
 from . import config
 from .common import format_exception
 from .common import format_method_exception
+from .common import topics_to_bytes
 from .common import unbound_method
 from .common import validate_handler
 from .common import LogLevel
@@ -646,7 +647,7 @@ class Agent():
         self.register(socket, server_address, alias, handler)
         # SUB sockets are a special case
         if kind == 'SUB':
-            self.subscribe(server_address, handler)
+            self._subscribe(server_address, handler)
         return server_address
 
     def _bind_channel(self, kind, alias=None, handler=None, addr=None,
@@ -782,7 +783,7 @@ class Agent():
         if client_address.kind == 'SUB':
             if not alias:
                 alias = client_address
-            self.subscribe(alias, handler)
+            self._subscribe(alias, handler)
         return client_address
 
     def _connect_channel(self, channel, alias=None, handler=None):
@@ -867,10 +868,7 @@ class Agent():
         uuid = unique_identifier()
         topic_handlers = {}
         if isinstance(handler, dict):
-            for key, value in handler.items():
-                if isinstance(key, str):
-                    key = key.encode()
-                topic_handlers[channel.uuid + key] = value
+            topic_handlers = topics_to_bytes(handler, uuid=channel.uuid)
         else:
             topic_handlers[channel.uuid] = handler
         topic_handlers[uuid] = self._handle_async_requests
@@ -914,7 +912,7 @@ class Agent():
         else:
             handler(self, response)
 
-    def subscribe(self, alias: str, handlers: Dict[Union[bytes, str], Any]):
+    def _subscribe(self, alias: str, handlers: Dict[Union[bytes, str], Any]):
         """
         Subscribe the agent to another agent.
 
@@ -930,12 +928,8 @@ class Agent():
         """
         if not isinstance(handlers, dict):
             handlers = {'': handlers}
-        # Convert all topics to bytes
-        curated_handlers = {}
-        for topic, value in handlers.items():
-            if isinstance(topic, str):
-                topic = topic.encode()
-            curated_handlers[topic] = value
+
+        curated_handlers = topics_to_bytes(handlers)
         # Subscribe to topics
         for topic in curated_handlers.keys():
             self.socket[alias].setsockopt(zmq.SUBSCRIBE, topic)
diff --git a/osbrain/common.py b/osbrain/common.py
index 14de969..16d4fd1 100644
--- a/osbrain/common.py
+++ b/osbrain/common.py
@@ -6,6 +6,9 @@ from threading import Event
 from threading import Thread
 import traceback
 from uuid import uuid4
+from typing import Any
+from typing import Dict
+from typing import Union
 
 from . import config
 
@@ -47,6 +50,33 @@ def format_method_exception(error, method, args, kwargs):
     return type(error)(message)
 
 
+def topics_to_bytes(handlers: Dict[Union[bytes, str], Any], uuid: bytes = b''):
+    '''
+    Given some pairs topic/handler, leaves them prepared for making the actual
+    ZeroMQ subscription.
+
+    Parameters
+    ----------
+    handlers
+        Contains pairs "topic - handler".
+    uuid
+        uuid of the SYNC_PUB/SYNC_SUB channel (if applies). For normal
+        PUB/SUB communication, this should be `b''`.
+
+    Returns
+    -------
+    Dict[bytes, Any]
+    '''
+    curated_handlers = {}
+
+    for topic, value in handlers.items():
+        if isinstance(topic, str):
+            topic = topic.encode()
+        curated_handlers[uuid + topic] = value
+
+    return curated_handlers
+
+
 def validate_handler(handler, required):
     '''
     Raises a ValueError exception when a required is handler but not present.


Test patch:
diff --git a/osbrain/tests/test_common.py b/osbrain/tests/test_common.py
index 3626fd0..55216ae 100644
--- a/osbrain/tests/test_common.py
+++ b/osbrain/tests/test_common.py
@@ -6,6 +6,7 @@ import pytest
 from osbrain.agent import TOPIC_SEPARATOR
 from osbrain.common import unique_identifier
 from osbrain.common import LogLevel
+from osbrain.common import topics_to_bytes
 
 
 def test_unique_identifier():
@@ -30,3 +31,34 @@ def test_loglevel():
     # Invalid initialization
     with pytest.raises(ValueError):
         LogLevel('FOO')
+
+
+def test_topics_to_bytes_without_uuid():
+    """
+    Test topics_to_bytes basic usage, without specifying a uuid.
+    """
+    handlers = {'topic_one': 'handler', b'topic_two': 'handler'}
+
+    curated = topics_to_bytes(handlers)
+    assert isinstance(curated, dict)
+
+    for k, v in curated.items():
+        assert isinstance(k, bytes)
+        assert k.startswith(b'topic_')
+        assert v == 'handler'
+
+
+@pytest.mark.parametrize('uuid', [b'', b'U'])
+def test_topics_to_bytes_with_uuid(uuid):
+    """
+    Test topics_to_bytes basic usage, specifying different uuid's.
+    """
+    handlers = {'topic_one': 'handler', b'topic_two': 'handler'}
+
+    curated = topics_to_bytes(handlers, uuid=uuid)
+    assert isinstance(curated, dict)
+
+    for k, v in curated.items():
+        assert isinstance(k, bytes)
+        assert k.startswith(uuid + b'topic_')
+        assert v == 'handler'