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.

Sign up for GitHub

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

Jump to bottom

fix!: realtime v2 #178

Merged
merged 54 commits into from
Aug 16, 2024
Merged

fix!: realtime v2 #178

merged 54 commits into from
Aug 16, 2024

Conversation

grdsdev
Copy link
Contributor

@grdsdev grdsdev commented Aug 8, 2024 •
edited
Loading

What kind of change does this PR introduce?

THIS PR HAS BREAKING CHANGES!

This PR provides several fixes for realtime, including:

  • Broadcast
  • Postgres Changes
  • Presence

Breaking Changes

TBD

@grdsdev grdsdev marked this pull request as ready for review August 9, 2024 14:13
grdsdev
grdsdev commented Aug 9, 2024
View reviewed changes
realtime/channel.py Outdated Show resolved Hide resolved
@juancarlospaco
Copy link
Contributor

I had to use **kargs on the callbacks

I think it is Ok there in this context,
You can also use *args and use the arguments passed by their names.

juancarlospaco
juancarlospaco reviewed Aug 9, 2024
View reviewed changes
realtime/presence.py Outdated Show resolved Hide resolved
@grdsdev
Copy link
Contributor Author

grdsdev commented Aug 9, 2024

I had to use **kargs on the callbacks

I think it is Ok there in this context, You can also use *args and use the arguments passed by their names.

Thanks, *args makes usage better I think.

grdsdev
grdsdev commented Aug 9, 2024
View reviewed changes
Makefile
@@ -13,7 +13,7 @@ tests_pre_commit:
run_tests: tests

tests_only:
poetry run pytest --cov=./ --cov-report=xml --cov-report=html -vv
Copy link
Contributor Author

Choose a reason for hiding this comment

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

it was failing on CI, don't know why, need to check it after.

Copy link
Collaborator

@J0 J0 Aug 16, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Maybe coverage requirements or missing plugin? In either case I think should be fine to do without

As an aside probably can consider moving towards using poetry scripts in the future when time frees up

@grdsdev grdsdev changed the title fix: realtime v2 fix!: realtime v2 Aug 11, 2024
grdsdev
grdsdev commented Aug 12, 2024
View reviewed changes
tests/test_connection.py Outdated Show resolved Hide resolved
filipecabaco
filipecabaco approved these changes Aug 12, 2024
View reviewed changes
Copy link
Contributor

@filipecabaco filipecabaco left a comment

Choose a reason for hiding this comment

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

not as versed in Python as I wished but the functionality and API seems sane and close to other libs 🔥

I also have a question on what will the supabase-py usage look like but probably that will be handled in a PR there.

usage.py Outdated Show resolved Hide resolved
juancarlospaco
juancarlospaco approved these changes Aug 12, 2024
View reviewed changes
Copy link
Contributor

@juancarlospaco juancarlospaco left a comment

Choose a reason for hiding this comment

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

👍

@grdsdev grdsdev mentioned this pull request Aug 13, 2024
Merged
juancarlospaco
juancarlospaco approved these changes Aug 13, 2024
View reviewed changes
Copy link
Contributor

@juancarlospaco juancarlospaco left a comment

Choose a reason for hiding this comment

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

LGTM

grdsdev
grdsdev commented Aug 15, 2024
View reviewed changes
pyproject.toml

[tool.poetry.group.dev.dependencies]
black = ">=23.11,<25.0"
isort = "^5.12.0"
pre-commit = "^3.5.0"
pytest-cov = "^5.0.0"
unasync-cli = { git = "https://github.com/supabase-community/unasync-cli.git", branch = "main" }
Copy link
Contributor Author

Choose a reason for hiding this comment

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

TODO: fix before merge

juancarlospaco
juancarlospaco reviewed Aug 15, 2024
View reviewed changes
README.md Outdated Show resolved Hide resolved
J0
J0 reviewed Aug 16, 2024
View reviewed changes
README.md Outdated Show resolved Hide resolved
J0
J0 reviewed Aug 16, 2024
View reviewed changes
README.md Outdated Show resolved Hide resolved
J0
J0 reviewed Aug 16, 2024
View reviewed changes
example/app.py Outdated Show resolved Hide resolved
J0
J0 reviewed Aug 16, 2024
View reviewed changes
example/app.py Outdated Show resolved Hide resolved
J0
J0 reviewed Aug 16, 2024
View reviewed changes
pyproject.toml Outdated Show resolved Hide resolved
J0
J0 reviewed Aug 16, 2024
View reviewed changes
realtime/_async/client.py Outdated Show resolved Hide resolved
J0
J0 approved these changes Aug 16, 2024
View reviewed changes
Copy link
Collaborator

@J0 J0 left a comment

Choose a reason for hiding this comment

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

Nice work

@grdsdev grdsdev merged commit 981a5d0 into master Aug 16, 2024
6 of 7 checks passed
@grdsdev grdsdev deleted the fix/realtime-v2 branch August 16, 2024 10:01
@github-actions github-actions bot mentioned this pull request Aug 16, 2024
Merged
timkpaine
timkpaine reviewed Aug 16, 2024
View reviewed changes
pyproject.toml
websockets = ">=11,<13"
python-dateutil = "^2.8.1"
typing-extensions = "^4.12.2"
asyncio = "^3.4.3"
Copy link
Contributor

Choose a reason for hiding this comment

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

why is this added? this is not a relevant package since python 3.3?

This was referenced Aug 19, 2024
Closed
Closed
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@juancarlospaco juancarlospaco juancarlospaco approved these changes

@filipecabaco filipecabaco filipecabaco approved these changes

@timkpaine timkpaine timkpaine left review comments

@J0 J0 J0 approved these changes

@mansueli mansueli Awaiting requested review from mansueli

@silentworks silentworks Awaiting requested review from silentworks

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@grdsdev @juancarlospaco @filipecabaco @timkpaine @J0