Skip to content

Move RPC handlers to room - #395

Open
typester wants to merge 17 commits into
mainfrom
typester/rpc-update
Open

Move RPC handlers to room#395
typester wants to merge 17 commits into
mainfrom
typester/rpc-update

Conversation

@typester

Copy link
Copy Markdown
Contributor
@typester typester changed the title WIP: 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 bcherry and theomonnom March 13, 2025 18:22
Comment thread livekit-rtc/livekit/rtc/participant.py
@@ -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
Copy Markdown
Contributor 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
Copy Markdown
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?

@davidzhao davidzhao left a comment

Copy link
Copy Markdown
Member

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

Comment thread examples/rpc_deprecated.py Outdated
Comment thread livekit-rtc/livekit/rtc/participant.py

@davidzhao davidzhao left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

lgtm

Comment thread livekit-rtc/setup.py
"protobuf>=4.25.0",
"types-protobuf>=3",
"aiofiles>=24",
"deprecated>=1.2.18",

@theomonnom theomonnom Mar 14, 2025

Copy link
Copy Markdown
Member

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
Copy Markdown
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
Copy Markdown
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

4 participants