-
Notifications
You must be signed in to change notification settings - Fork 14
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
RestClient subscriptions #48
Conversation
In general, the CLI is a good candidate for refactoring/cleaning up. While it works, and the click decorator does seem elegant, it is also a jumble and getting near to spaghetti code (in a way). While this is not pressing now (probably the last thing we do), we should start thinking about this. |
currencies = cls._validate_parameter('currencies', currencies) | ||
channels = cls._validate_parameter('channels', channels) | ||
def subscribe(self, assets, currencies, channels, exchange=None, | ||
interval=1.0): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is interval in seconds? If so, perhaps we can call it interval_seconds
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes it is in seconds
handlers=self._get_handlers(), | ||
) | ||
# FIXME: find a better way to do this | ||
subscription.listener = self._listener(subscription, interval=interval, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you ping me on WA and detail ideas about what is a better way to do this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure. I think it will come out in the wash once we think carefully about how to share the sockets across subscriptions.
def _get_handlers(cls): | ||
return [getattr(cls, attr) for attr in dir(cls) | ||
if callable(getattr(cls, attr)) | ||
and attr.startswith('parse_')] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The node community uses the prefix on
. So for example, on_connect
and on_trade
. It seems like it is a very short prefix and quite descriptive of the purpose. What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So some of the python websocket client classes do this too. I see the point. If we go with the event handler publishing straight to the stream model then I think that sounds like a good convention, i.e. for the Websocket subscription handle_*() handlers. These ones just parse a message and return an event so slightly less appropriate here in my opinion but let me think about it for a bit and get back to you.
Adds ability to subscribe to RestClient updates (currently prices and tickers)