Add a possibility to create subscribers using a dictionary of configuration options #39
Conversation
Codecov Report
@@ Coverage Diff @@
## main #39 +/- ##
==========================================
+ Coverage 79.50% 80.36% +0.85%
==========================================
Files 15 15
Lines 1888 1991 +103
==========================================
+ Hits 1501 1600 +99
- Misses 387 391 +4
Continue to review full report at Codecov.
|
|
Found couple bugs when running tests locally. Fixing and adding some more tests. |
|
Ok, the bugs were fixed. I've also tested this with |
| def _get_subscriber_instance(settings): | ||
| addresses = settings['addresses'] | ||
| topics = settings.get('topics', '') | ||
| message_filter = settings.get('message_filter', None) | ||
| translate = settings.get('translate', False) | ||
|
|
||
| return Subscriber(addresses, topics=topics, message_filter=message_filter, translate=translate) | ||
|
|
||
|
|
||
| def _get_nssubscriber_instance(settings): | ||
| services = settings.get('services', '') | ||
| topics = settings.get('topics', _MAGICK) | ||
| addr_listener = settings.get('addr_listener', False) | ||
| addresses = settings.get('addresses', None) | ||
| timeout = settings.get('timeout', 10) | ||
| translate = settings.get('translate', False) | ||
| nameserver = settings.get('nameserver', 'localhost') or 'localhost' | ||
|
|
||
| return NSSubscriber( | ||
| services=services, | ||
| topics=topics, | ||
| addr_listener=addr_listener, | ||
| addresses=addresses, | ||
| timeout=timeout, | ||
| translate=translate, | ||
| nameserver=nameserver | ||
| ) |
There was a problem hiding this comment.
how about making these class method call eg from_dict_config?
There was a problem hiding this comment.
Umm, what? I don't understand.
There was a problem hiding this comment.
instead of calling _get_substriber_instance, you would call Subscriber.from_dict_config. This can be implemented lik this:
# In the Subscriber class definition
@classmethod
def from_dict_config(cls, settings):
addresses = settings['addresses']
topics = settings.get('topics', '')
message_filter = settings.get('message_filter', None)
translate = settings.get('translate', False)
return cls(addresses, topics=topics, message_filter=message_filter, translate=translate)There was a problem hiding this comment.
Testing that only relevant arguments are passed to the class init seems to be getting too complicate, so I'll keep these as separate methods.
There was a problem hiding this comment.
Can we make the tests simpler?
There was a problem hiding this comment.
- if the names of the kwargs in the
SubscriberorNSSubscriberconstructors changes, the tests you are referring to don't catch it, right?
There was a problem hiding this comment.
Damn, that's right. How about I change the tests so that I don't mock them, but neither do I assert anything. That way we see that the kwargs truly match. I'll just mock NSSubscriber.start() so that it isn't started for nothing.
There was a problem hiding this comment.
That wouldn't catch if new kwargs were added, but better that than the current version of the tests...
There was a problem hiding this comment.
- indeed, since the replacement of the calls to the
NSSubscriberwithcreate_subscriber_from_dict_config, the latter is tested end-to-end in behaviour with the tests you mention in your last comment. That means that the new/refactored code is effectively tested already.
The other tests that you mentionned earlier still only test implementation IMO. They rely on the fact that the new functions end up calling a given constructor, while we might change that to a unpickling (yaml loading) or class methods in the future. In other words, according to me, these tests are not useful and should be deleted.
That would then open for having class methods as mentioned earlier.
Similarly to #35 , this PR adds the possibility to configure the subscriber using a dictionary of config options. In this way it is possible to disable
nameserverconnection in theListenerContainerwhen it is not needed, or is otherwise undesirable.The subscriber class without
nameserverconnection is selected by settingnameserver=Falseand giving a non-empty list of connection addresses. In all other cases the defaultNSSubscriberwill be used.This PR is required by pytroll/pytroll-collectors#102