PTDM subscribes to relevant events only#590
Conversation
Codecov ReportBase: 0.00% // Head: 60.51% // Increases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## master #590 +/- ##
===========================================
+ Coverage 0 60.51% +60.51%
===========================================
Files 0 147 +147
Lines 0 7325 +7325
===========================================
+ Hits 0 4433 +4433
- Misses 0 2892 +2892
Flags with carried forward coverage won't be shown. Click here to find out more.
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
| if callback: | ||
| self.invoke_callback_func_if_exists(callback, message.parameters) | ||
|
|
||
| def invoke_callback_func_if_exists(self, callback, params=list()): |
There was a problem hiding this comment.
i'm not sure but this looks like it would have the same issue as using [] as a default, that the same list is actually reused each time. probably not an issue since you don't mutate it, but I still think it's better practice to use None as the default and then have like params = list() if params is None else params
There was a problem hiding this comment.
also, not new things but:
- the naming of this function is confusing me - it doesn't check wether the callback exists it just calls it?
- below is
if params == list()just a weird way to check if the list is empty? usually people check the length?
| self._zmq_socket.setsockopt_string(zmq.SUBSCRIBE, "") | ||
|
|
||
| for message_id in self._callback_funcs.keys(): | ||
| self._zmq_socket.setsockopt(zmq.SUBSCRIBE, str(message_id).encode()) |
| callback() | ||
| else: | ||
| func(params) | ||
| callback(params) |
There was a problem hiding this comment.
codecov seems to say this method isn't tested at all - maybe you could quickly add one where the callback is actually called?
angusjfw
left a comment
angusjfw
left a comment
There was a problem hiding this comment.
the message filtering thing seems to make sense and be correct to me, but it seems like you have removed parts of the test that actually check the callbacks work
jcapona
force-pushed
the
ptdm-subscribes-to-relevant-events
branch
2 times, most recently
from
bc36a83 to
6f30d65
Compare
jcapona
force-pushed
the
ptdm-subscribes-to-relevant-events
branch
from
6f30d65 to
2873910
Compare
2 participants
Main changes
Improve
PTDMSubscribeClientstructure by only subscribing to the events that were marked as relevant according to the dictionary passed to theinitialisemethod. By doing this, overhead is reduced since it's not necessary to filter the received messages once they're received since now when that happens, we're 100% sure it's from an event we're interested in.Couldn't remove the polling since this is required to stop the
PTDMSubscribeClient's internal thread.Screenshots (feature, test output, profiling, dev tools etc)
[insert screenshots here]
Other notes (e.g. implementation quirks, edge cases, questions / issues)
Manual testing tips
Tag anyone who definitely needs to review or help