Asynchronous API Alternatives #135
Conversation
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @Maturin - I've reviewed your changes and they look great!
Here's what I looked at during the review
- 🟡 General issues: 2 issues found
- 🟢 Security: all looks good
- 🟡 Testing: 2 issues found
- 🟢 Complexity: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.
| # @ensure_connection is definitely nicer, but I don't know if it is also | ||
| # working for asynchronous functions. | ||
| if not self.connected: | ||
| raise NotConnectedError(self.listen_async.__name__) |
There was a problem hiding this comment.
suggestion (code_refinement): Consider adding a more descriptive error message.
Providing a more detailed error message can help in debugging and understanding the context of the error when it occurs.
| raise NotConnectedError(self.listen_async.__name__) | |
| raise NotConnectedError(f"Failed to connect using {self.listen_async.__name__} in realtime/connection.py. Ensure the connection is established before invoking this method.") |
There was a problem hiding this comment.
I have two questions regarding this:
- This basically the same exception and exception message as in the
ensure_connectionfunction. - As written in my comment, the question is if should create also an ensure_connection_async function/decorator. Using simply
ensure_connectionwas marked as an error in PyCharm.
| async def listen_async(self) -> None: | ||
| """ | ||
| Wrapper for async def _listen() and async def _keep_alive() to expose an async interface. | ||
| :return: None | ||
| """ | ||
| # @ensure_connection is definitely nicer, but I don't know if it is also | ||
| # working for asynchronous functions. | ||
| if not self.connected: | ||
| raise NotConnectedError(self.listen_async.__name__) | ||
|
|
There was a problem hiding this comment.
issue (testing): Missing tests for the new asynchronous methods.
The PR introduces new asynchronous methods such as listen_async and connect_async, but there are no corresponding tests to verify their behavior. It's crucial to ensure these methods handle asynchronous operations correctly and maintain the expected state of the connection.
| async def join_async(self) -> None: | ||
| """ | ||
| Wrapper for async def _join(). | ||
| :return: None | ||
| """ | ||
| await self._join() |
There was a problem hiding this comment.
issue (testing): Tests needed for join_async method.
The addition of join_async in Channel class also requires tests to ensure it integrates properly with the existing asynchronous infrastructure and behaves as expected under various conditions.
realtime/connection.py
Outdated
| @@ -156,4 +156,4 @@ def summary(self) -> None: | |||
| """ | |||
| for topic, chans in self.channels.items(): | |||
| for chan in chans: | |||
| print(f"Topic: {topic} | Events: {[e for e, _ in chan.callbacks]}]") | |||
| print(f"Topic: {topic} | Events: {[e for e, _ in chan.listeners]}]") | |||
There was a problem hiding this comment.
This bug fix is not really related to the feature request. But I simply did it, while working on it. If you want, I can create a new bug ticket and solve it there.
|
This pull request has to be merged with PR #136. |
|
V2 supports async now, implemented in #178 |
What kind of change does this PR introduce?
Added a feature as described in #133
What is the current behavior?
#133
What is the new behavior?
Added the option to use the realtime library in an async programming environment.
Additional context
n/a