Conversation
Summary of ChangesHello @mesudip, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #3 +/- ##
==========================================
- Coverage 83.38% 82.69% -0.69%
==========================================
Files 28 30 +2
Lines 1294 1387 +93
Branches 137 153 +16
==========================================
+ Hits 1079 1147 +68
- Misses 170 189 +19
- Partials 45 51 +6 ☔ View full report in Codecov by Sentry. |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces significant improvements to the RPC interface, most notably adding support for call-specific logging, progress reporting, and cancellation via the new RpcCallHandle. The refactoring also enhances type safety by making RpcServer generic. The documentation has been updated to reflect these new capabilities. While the core changes are excellent, there are a few areas that need attention. The example for SSH RPC is now broken due to the API changes and needs to be updated. Additionally, there are some minor issues in the documentation and a few inconsistencies in test helper method signatures that should be addressed for clarity and correctness.
|
|
||
| # Create RPC Client using the SSH pipe | ||
| client = RpcClient(json_pipe) | ||
| client = RpcInitClient(json_pipe) |
There was a problem hiding this comment.
The change to RpcInitClient (which is now RpcV1) breaks this example. The new RpcV1 API does not have set_on_log, request, or close methods directly on the client instance. The example needs to be updated to use the new RpcCallHandle for making calls and handling logs, and pipe.terminate() for closing the connection. A working example is crucial for users to understand and adopt the new API.
| - **Timeouts**: Default 30s. Set via `rpc.set_timeout(ms)`. | ||
| - **Context**: `RpcCallContext` in handlers provides metadata/logging. | ||
| - **Logging**: Controlled via `cbor_rpc.RpcLogger`. | ||
| m No newline at end of file |
There was a problem hiding this comment.
There seems to be a stray 'm' character at the end of the file. Please remove it.
| 2. **`pipeline("data", handler)`**: Used for serial processing. Handlers are awaited in order. If a pipeline handler throws an error, it stops the chain and emits an `"error"`. | ||
| 1. **`on("data", handler)`**: Simple pub/sub. The handler is called whenever data arrives. If it's a coroutine, it's run in the background. |
There was a problem hiding this comment.
The numbering in this list is incorrect. It goes from 2 to 1. It should be sequential.
| 2. **`pipeline("data", handler)`**: Used for serial processing. Handlers are awaited in order. If a pipeline handler throws an error, it stops the chain and emits an `"error"`. | |
| 1. **`on("data", handler)`**: Simple pub/sub. The handler is called whenever data arrives. If it's a coroutine, it's run in the background. | |
| 1. **`pipeline("data", handler)`**: Used for serial processing. Handlers are awaited in order. If a pipeline handler throws an error, it stops the chain and emits an `"error"`. | |
| 2. **`on("data", handler)`**: Simple pub/sub. The handler is called whenever data arrives. If it's a coroutine, it's run in the background. |
| def on_progress(self, callback: Callable[[Any, Any], None]) -> "RpcCallHandle": | ||
| """ | ||
| Register a callback for progress updates. | ||
| Callback signature: (value: Any, metadata: Any) | ||
| """ | ||
| self._progress_listeners.append(callback) |
There was a problem hiding this comment.
For consistency with on_log, the on_progress method should also return self to allow for method chaining. This improves the usability of the RpcCallHandle API.
| def on_progress(self, callback: Callable[[Any, Any], None]) -> "RpcCallHandle": | |
| """ | |
| Register a callback for progress updates. | |
| Callback signature: (value: Any, metadata: Any) | |
| """ | |
| self._progress_listeners.append(callback) | |
| def on_progress(self, callback: Callable[[Any, Any], None]) -> "RpcCallHandle": | |
| """ | |
| Register a callback for progress updates. | |
| Callback signature: (value: Any, metadata: Any) | |
| """ | |
| self._progress_listeners.append(callback) | |
| return self |
|
|
||
| self._resolve_result = resolve_result | ||
|
|
||
| async def on_data(data: List[Any]) -> None: |
There was a problem hiding this comment.
The on_data method is quite long and handles all protocol messages in a single block. To improve readability and maintainability, consider refactoring it by breaking it down into smaller, more focused helper methods for each protocol or message type (e.g., _handle_rpc_call, _handle_stream_message, _handle_event), similar to the previous _handle_proto_* structure.
| class RpcClient(RpcInitClient): | ||
| @abstractmethod | ||
| def get_id(self) -> str: | ||
| pass |
There was a problem hiding this comment.
The renaming of RpcAuthorizedClient to RpcClient and RpcClient to RpcInitClient could be confusing. While the new structure is logical, it might be worth adding a note in the documentation or PR description about this change to avoid confusion for developers familiar with the previous API.
| @pytest.fixture(params=["stdio", "unix_local", "unix_ssh"]) | ||
| async def rpc_client(request, get_stdio_server_script_path, ssh_server): | ||
| if request.param == "stdio": | ||
| async with _stdio_pipe_ctx(get_stdio_server_script_path) as pipe: | ||
| yield RpcV1.read_only_client(pipe) | ||
| elif request.param == "unix_local": | ||
| async with _unix_local_pipe_ctx() as pipe: | ||
| yield RpcV1.read_only_client(pipe) | ||
| elif request.param == "unix_ssh": | ||
| async with _unix_ssh_pipe_ctx(ssh_server) as pipe: | ||
| yield RpcV1.read_only_client(pipe) | ||
| else: | ||
| raise ValueError(f"Unknown param: {request.param}") |
There was a problem hiding this comment.
Using a parametrized fixture to run tests against multiple transport layers (stdio, unix local, unix ssh) is an excellent way to ensure broad compatibility and reduce code duplication. This is a great improvement to the test suite's structure and effectiveness.
| def handle_method_call(self, method: str, args: List[Any]) -> Any: | ||
| raise Exception("Event-only RPC") |
There was a problem hiding this comment.
The signature of handle_method_call in the EventRpcHelper class does not match the one in its superclass RpcV1. It's missing the context: RpcCallContext parameter. Please update the signature to maintain consistency with the base class.
| def handle_method_call(self, method: str, args: List[Any]) -> Any: | |
| raise Exception("Event-only RPC") | |
| def handle_method_call(self, context: RpcCallContext, method: str, args: List[Any]) -> Any: | |
| raise Exception("Event-only RPC") |
2 participants
No description provided.