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

Move RPC handlers to room #395

Open
wants to merge 17 commits into
base: main
Choose a base branch
from
Open

Move RPC handlers to room #395

wants to merge 17 commits into from

Conversation

typester
Copy link
Member

@typester typester changed the title WIP: Move RPC handlers to room Move RPC handlers to room Mar 13, 2025
@typester typester marked this pull request as ready for review March 13, 2025 18:22
@typester typester requested review from theomonnom and bcherry March 13, 2025 18:22
@@ -483,7 +564,7 @@ def _on_rpc_method_invocation(self, rpc_invocation: RpcMethodInvocationEvent):

if rpc_invocation.local_participant_handle == self._local_participant._ffi_handle.handle:
Copy link
Member Author

Choose a reason for hiding this comment

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

Is this local_participant check still needed?

Copy link
Contributor

Choose a reason for hiding this comment

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

i think this is here in the case that you connect to multiple rooms simultaneously?

Copy link
Member

@davidzhao davidzhao left a comment

Choose a reason for hiding this comment

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

lg, just a couple of nits

Copy link
Member

@davidzhao davidzhao left a comment

Choose a reason for hiding this comment

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

lgtm

"protobuf>=4.25.0",
"types-protobuf>=3",
"aiofiles>=24",
"deprecated>=1.2.18",
Copy link
Member

@theomonnom theomonnom Mar 14, 2025

Choose a reason for hiding this comment

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

nit: do we want to add a new package just for deprecation?

let's just log a warning no?

Copy link
Member

Choose a reason for hiding this comment

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

just saw this comment, either way #395 (comment) ...

Copy link
Member

Choose a reason for hiding this comment

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

we can log a warning too! I don't have a preference.. the only preference is making sure users know they are using a deprecated function

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.

4 participants