Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

Comments

Close side panel

Move RPC handlers to room#395

Open
typester wants to merge 17 commits intomainlivekit/python-sdks:mainfrom
typester/rpc-updatelivekit/python-sdks:typester/rpc-updateCopy head branch name to clipboard
Open

Move RPC handlers to room#395
typester wants to merge 17 commits intomainlivekit/python-sdks:mainfrom
typester/rpc-updatelivekit/python-sdks:typester/rpc-updateCopy head branch name to clipboard

Conversation

@typester
Copy link
Contributor

@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 bcherry and theomonnom March 13, 2025 18:22
livekit-rtc/livekit/rtc/participant.py Show resolved Hide resolved
@@ -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
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
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

examples/rpc_deprecated.py Outdated Show resolved Hide resolved
livekit-rtc/livekit/rtc/participant.py Show resolved Hide resolved
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

Morty Proxy This is a proxified and sanitized view of the page, visit original site.