fix: address Gemini Code Assist review feedback from PRs #1-3#19
fix: address Gemini Code Assist review feedback from PRs #1-3#19
Conversation
- SubprocessPlugin: pass **kwargs through _which_interceptor to _handle_which (shutil.which mode/path args were silently dropped) - SubprocessPlugin: raise TypeError on string commands in _handle_run instead of silently splitting into characters - Replace loop-and-break plugin lookups with idiomatic next() in subprocess, popen, and proxy code - Extract _push_cm() helper in _BigfootModule to deduplicate __enter__/__aenter__ sandbox creation logic Bump version to 0.10.1.
Summary of ChangesHello, 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 addresses several pieces of feedback from previous code reviews, focusing on improving the robustness, clarity, and idiomatic nature of 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
|
![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 addresses several pieces of feedback, improving code quality and fixing bugs. The changes include using more idiomatic Python constructs like next() for lookups, refactoring duplicated code into a helper function, and adding robustness to the SubprocessPlugin by handling string commands and passing through keyword arguments. My review includes one suggestion to further improve the SubprocessPlugin by recording the passed-through keyword arguments to allow for more precise test assertions.
| ) | ||
|
|
||
| def _handle_which(self, name: str) -> str | None: | ||
| def _handle_which(self, name: str, **kwargs: Any) -> str | None: # noqa: ANN401 |
There was a problem hiding this comment.
While it's great that **kwargs are now passed through from the interceptor, they are currently ignored in this handler. This could lead to subtle bugs in tests where arguments like path are expected to influence the behavior of shutil.which.
To make this mock more robust, I suggest recording these kwargs in the interaction details. This would make the mock's behavior more transparent and allow tests to assert that which was called with specific options.
You would need to update the details creation in this function, and then update assertable_fields to include "kwargs" for which interactions.
For example:
# In _handle_which
details = {"name": name, "returns": config.returns, "kwargs": kwargs}
# In assertable_fields
if interaction.source_id == _SOURCE_WHICH:
return frozenset({"name", "returns", "kwargs"})- Add explicit type annotations to avoid Returning Any errors - Remove unused type: ignore comments - Add import-untyped ignores for psycopg2 and asyncpg (no stubs available) - Add assignment ignores for aiohttp URL vs str type mismatch
|
Re: Gemini's suggestion to record |
1 participant
Summary
Addresses all outstanding Gemini Code Assist review feedback from PRs #1, #2, and #3.
High priority fixes
_which_interceptornow passes**kwargs(mode, path) to_handle_whichinstead of silently dropping them_handle_runraisesTypeErroron string commands instead of silently splitting into characters ("ls"->['l', 's'])Medium priority fixes
next()lookups: Replaced loop-and-break plugin lookup patterns withnext()expressions in subprocess, popen, and proxy code_BigfootModulecontext manager: Extracted_push_cm()helper to eliminate duplicated sandbox creation logic in__enter__and__aenter__Already addressed (no changes needed)
wait()idempotency (PR Add StateMachinePlugin infrastructure and stateful protocol plugins (v0.4.0) #2 high): already guarded byif self.returncode is not None_plugins[-1]in docs (PR Add StateMachinePlugin infrastructure and stateful protocol plugins (v0.4.0) #2 medium): already uses proxy pattern__getattr__boilerplate (PR Add StateMachinePlugin infrastructure and stateful protocol plugins (v0.4.0) #2 medium): already uses_get_or_create_pluginhelperheaders/bodyparams (PR feat: full assertion certainty for all plugins (v0.5.0) #4 medium): intentionally rejected (DRY violation)Version bump
0.10.1
Test plan
python -m pytest tests/ examples/ -x -q-- 773 passedruff check src/ tests/ examples/-- clean