-
Notifications
You must be signed in to change notification settings - Fork 15
Support Websockets API #8
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
base: master
Are you sure you want to change the base?
Conversation
* Support sandbox mode * Add DEMO BASE Url * Add DEMO BASE wss Url
|
Wow! That's amazing. @radovsky1 I am so thankful to you. I wanted to add this functionality for so long but was pretty busy with full time job. I am so inspired by your PR. Will review it shortly |
sann05
left a comment
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.
Commonly it's pretty big pull request, so I will merge him after the fixes but, I have to add test for new code before pushing to pip
| def test_get_server_time(self, monkeypatch): | ||
| self.client.get_server_time() | ||
| self.mock_requests.assert_called_once_with( | ||
| CurrencyComConstants.SERVER_TIME_ENDPOINT |
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.
Tests do not pass. You should pass constants as an object not a class.
It would be better to replace CurrencyComConstants with self.client.constants
|
|
||
|
|
||
| class Client(object): | ||
| class CurrencycomClient: |
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.
To avoid breaking anyone who works with this API I would add Client inherited by CurrencycomClient with deprication warning
|
|
||
| def _get_reconnect_wait(self, attempts): | ||
| expo = 2 ** attempts | ||
| return round(random() * min(self.MAX_RECONNECT_SECONDS, expo - 1) + 1) |
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.
Why do you do random there?
| self._conn = None | ||
| self._connect_id = None | ||
| self._socket = None | ||
| self._request = { |
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.
Let's move this var to send_message as it shouldn't be stored across the object
| @@ -0,0 +1,241 @@ | |||
| import asyncio | |||
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.
Let's avoid adding packages with the same name as default asyncio package
After using the library for a long time, I came to the conclusion that Websocket support is needed to ensure updating of market data, depth market data, OHLC data and so on.
Implemented CurrencycomHybridClient to support REST API as well as Websockets subscribtions.