You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
Hey @Taiwo-Sh, thanks for the contribution! The overall direction makes sense. I have some feedback after reviewing the code:
Suggestion: use **endpoint_kwargs instead of a premade instance
The main concern with accepting a pre-built WebsocketRPCEndpoint is that PubSubEndpoint needs to register its on_disconnect handler (for subscription cleanup), and the only way to do that on an existing instance is by mutating its private _on_disconnect / _on_connect lists. This creates tight coupling to RPC internals and introduces risk — e.g. the rpc_channel_get_remote_id value can get out of sync between the two objects.
A simpler approach that still solves #96: have PubSubEndpoint forward extra kwargs to WebsocketRPCEndpoint:
This avoids all the new branching/validation logic, private attribute access, and callback merging — PubSubEndpoint keeps full ownership of the endpoint lifecycle.
Other notes
The docstring for the endpoint param is truncated (ends with `met)
setup_server_with_no_methods_endpoint is identical to setup_server_with_premade_endpoint
test_premade_endpoint_custom_methods_preserved doesn't actually call or verify the custom method
test_premade_endpoint_with_methods_and_notifier doesn't verify the notifier identity is preserved
Happy to discuss — and thanks again for working on this!
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR addresses #96
WebsocketRPCEndpointwith appropriate methods type can now be used to create aPubSubEndpoint.