-
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
Replaces 'symbol' with 'asset' and 'currency' in Events for consistency #56
Conversation
This fixes issue #52 |
Since symbol differs by Exchange, we decompose this into asset and currency so that these are consistent across Exchanges.
return subscriptions | ||
|
||
def _subscribe(self, asset, currency, channel, exchange=None, | ||
interval=1.0): | ||
if self._websocket_client_class is not None: |
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.
In the implementation below, if WS client is defined, it will ignore the Rest client class, implying that if you have both WS client and Rest Client defined, and (for argument sake) the subscribe you are interested in is defined on the Rest client, it will get ignored here?
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 agree with you. We would need to define a mechanism to resolve that. I'll create an issue for that. This has existed in the codebase for a while and doesn't need to be fixed in this PR.
|
||
def subscribe(self, asset, currency, channel, interval=1.0, exchange=None): | ||
exchange = exchange if exchange else self.exchange | ||
# FIXME: Remove channel_info or make it more generic |
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.
Try to make it more generic...
Since symbol differs by Exchange, we decompose this into asset and currency so
that these are consistent across Exchanges.