Review
netrc
unless there is some import magic involved, but I doubt it, since netrc is stdlib, the imported netrc module is not used, and there is a typo
log.exception('Cannot load netrc file: %s', netrc) =>
log.exception('Cannot load netrc file: %s', netrcfile)
Can't the following be simply done (see comment below about renaming netrc to netrcfile):
from safe_netrc import netrc
instead of:
from safe_netrc import netrc as _netrc
def _get_login(username, password, netrc, interactive, server):
netrc -> netrcfile, to be consistent with _get_login_default
Client
class LVAlertClient(..., netrc, ...) -> netrcfile ?
line 61, 63, 65, 67: no else after return,
_get_default_login should be called before the statement "if username is None and default_username is None", to avoid I/O overhead when username and password are defined.
line 134: async def callback(*args): slixmpp callbacks have a signature: async def callback(event), So it's better using event, instead of *args Moreover, since LVAlertClient callbacks (in listen) have a different signature, it is better to make them explicit.
line 57, 165, 253: f-strings!
line 178: def listen(self, callback): It looks more standard and flexible to have a method add_listener to handle several callbacks. In Pub/Sub terminology, the method would better be named add_message_handler. (A method remove_message_handler would be expected, too) _pubsub_publish (better named as _handle_message) would then call the callbacks in a loop
line 189: simplification of the part that stops the client
def start(self):
...
self._alive = self.loop.create_future()
try:
self.loop.run_until_complete(self._alive)
except CancelledError # from asyncio import CancelledError
pass
finally:
...
def stop(self):
...
self.loop.call_soon_threadsafe(self._alive.cancel)
and the method _stop is not necessary anymore
coroutines get_nodes, get_subscriptions, subscribe and unsubscribe
-
I don't really see a case to make these methods awaitable. They are meant to be called before the "listening" part, right ? Is it true that what is gained in terms of I/O latency by being able to run these methods asynchronously really outweighs a clearly better client user experience:
client = LVAlertClient(...) client.subscribe('cbc_gstlal', 'cbc_pycbc') print(client.get_subscriptions()) client.add_message_handler(message_handler) client.start()
NB: that would work only if the calls self.init_plugins() and self.connect() are move in the __init__
client constructor. Having a client fully initialized and connected upon instanciation looks a reasonable assumption to me.
- if these methods become regular non-awaitable ones, is there still a real world use case to call the low-level method add_event_handler. If not, it should be removed from the documentation.