Skip to content

Conversation

@radovsky1
Copy link

@radovsky1 radovsky1 commented Jul 27, 2022

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.

@sann05
Copy link
Owner

sann05 commented Jul 27, 2022

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

Copy link
Owner

@sann05 sann05 left a 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
Copy link
Owner

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:
Copy link
Owner

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)
Copy link
Owner

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 = {
Copy link
Owner

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
Copy link
Owner

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

Repository owner deleted a comment from Jcillo507 Feb 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants