Skip to content
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

RFC: realtime v2 #139

Open
wants to merge 13 commits into
base: master
Choose a base branch
from
Open

RFC: realtime v2 #139

wants to merge 13 commits into from

Conversation

mansueli
Copy link
Contributor

@mansueli mansueli commented May 6, 2024

What kind of change does this PR introduce?

Bug fix, feature, docs update, ...

What is the current behavior?

Please link any relevant issues here.

What is the new behavior?

Feel free to include screenshots if it includes visual changes.

Additional context

Add any other context or screenshots.

filipecabaco and others added 6 commits December 15, 2023 17:05
* Adds support for Realtime V2 by propagating the configuration on the first connect message.
* Adds example project for quick testing during development
@mansueli mansueli marked this pull request as ready for review May 6, 2024 17:52
@mansueli mansueli requested a review from a team as a code owner May 6, 2024 17:52
@mansueli mansueli changed the title Feat silly dx attemps RFC: realtime v2 May 6, 2024
Copy link

@sourcery-ai sourcery-ai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @mansueli - I've reviewed your changes and they look great!

Here's what I looked at during the review
  • 🟡 General issues: 1 issue found
  • 🟡 Security: 1 issue found
  • 🟢 Testing: all looks good
  • 🟢 Complexity: all looks good

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.

realtime/channel.py Outdated Show resolved Hide resolved
realtime/connection.py Show resolved Hide resolved
Comment on lines +187 to +189
raise Exception(
"Tried to subscribe multiple times. 'subscribe' can only be called a single time per channel instance"
)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

issue (code-quality): Raise a specific error instead of the general Exception or BaseException (raise-specific-error)

ExplanationIf a piece of code raises a specific exception type rather than the generic [`BaseException`](https://docs.python.org/3/library/exceptions.html#BaseException) or [`Exception`](https://docs.python.org/3/library/exceptions.html#Exception), the calling code can:
  • get more information about what type of error it is
  • define specific exception handling for it

This way, callers of the code can handle the error appropriately.

How can you solve this?

So instead of having code raising Exception or BaseException like

if incorrect_input(value):
    raise Exception("The input is incorrect")

you can have code raising a specific error like

if incorrect_input(value):
    raise ValueError("The input is incorrect")

or

class IncorrectInputError(Exception):
    pass


if incorrect_input(value):
    raise IncorrectInputError("The input is incorrect")

realtime/channel.py Outdated Show resolved Hide resolved
realtime/connection.py Show resolved Hide resolved
Comment on lines +117 to +122
if self.auto_reconnect:
logging.info("Retrying connection...")
await asyncio.sleep(5) # Wait before retrying
await self._connect() # Retry connection
else:
raise
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

issue (code-quality): We've found these issues:

realtime/presence.py Outdated Show resolved Hide resolved
realtime/transformers.py Outdated Show resolved Hide resolved
realtime/transformers.py Outdated Show resolved Hide resolved
realtime/transformers.py Outdated Show resolved Hide resolved
mansueli and others added 7 commits May 6, 2024 15:15
Co-authored-by: sourcery-ai[bot] <58596630+sourcery-ai[bot]@users.noreply.github.com>
Co-authored-by: sourcery-ai[bot] <58596630+sourcery-ai[bot]@users.noreply.github.com>
Co-authored-by: sourcery-ai[bot] <58596630+sourcery-ai[bot]@users.noreply.github.com>
Co-authored-by: sourcery-ai[bot] <58596630+sourcery-ai[bot]@users.noreply.github.com>
Co-authored-by: sourcery-ai[bot] <58596630+sourcery-ai[bot]@users.noreply.github.com>
Co-authored-by: sourcery-ai[bot] <58596630+sourcery-ai[bot]@users.noreply.github.com>
@silentworks
Copy link
Contributor

@mansueli this PR will need to be updated as some other PRs got merged in which are now causing merge conflicts with this one.

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.

None yet

3 participants