feat: Add shell completion support (bash, zsh, fish, powershell, elvish)#501
feat: Add shell completion support (bash, zsh, fish, powershell, elvish)#501dumko2001 wants to merge 5 commits intogoogleworkspace:mainfrom
Conversation
🦋 Changeset detectedLatest commit: 4b53399 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
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 significantly enhances the command-line interface by introducing robust shell completion capabilities. Users can now generate shell-specific scripts to enable dynamic suggestions for commands, subcommands, and flags, greatly improving usability and discoverability. The implementation uses a bridge approach, allowing the CLI to provide context-aware completions across various popular shells. Highlights
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
|
There was a problem hiding this comment.
Code Review
This pull request introduces shell completion support for various shells, which is a great feature for usability. The implementation uses a bridge script approach with a hidden __complete command, which is a solid pattern for dynamic CLIs. I've found one high-severity issue with the argument parsing for the new completion command, which can lead to incorrect behavior or panics when flags like --help are used. My suggestion addresses this by using the existing clap parser, making the implementation more robust and consistent.
|
Thanks for the suggestion! I've refactored the completion command parsing to use the existing |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces shell completion support for various shells, which is a great feature. The implementation uses a bridge script and a hidden __complete command to provide dynamic completions, which is a solid approach. The new completion.rs module is well-structured, and the integration into main.rs with the build_static_cli refactoring is clean.
I have two main points of feedback:
- In
src/completion.rs, there's a hardcoded list of static commands that creates a maintenance burden. I've suggested a more dynamic approach to avoid this. - The PR also includes a downgrade of the
ratatuidependency from version0.30.0to0.29.0. This is a significant change that seems unrelated to shell completion and should ideally be in a separate pull request to avoid scope creep.
Overall, the core feature is well-implemented. Addressing these points will improve the long-term maintainability of the code.
|
I've refactored the static command detection in |
|
/gemini review |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces shell completion support for various shells, a valuable feature for the CLI. The implementation uses a bridge command __complete to dynamically generate completions, which is a suitable approach for a CLI with a dynamic command surface. My review identified a critical issue in the argument parsing logic within the completion handler. The current implementation does not correctly handle flags that take values, causing completions to fail in common scenarios. I have provided a detailed comment and a code suggestion to address this bug.
|
Excellent catch! I've updated the argument parsing logic to correctly identify flags that take values and consume them. This ensures that commands like |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces shell completion support, which is a valuable feature for the CLI. The implementation uses a bridge command (__complete) to handle dynamic completions, a suitable approach for this tool. However, the argument parsing logic within the completion handler has a critical flaw: it doesn't correctly process flags that take values. This leads to incorrect completion suggestions in many common scenarios. I've provided a detailed comment with a suggested fix to improve the robustness of the argument parsing.
|
I've implemented the more robust flag parsing logic as suggested. The completer now correctly identifies flags using their |
There was a problem hiding this comment.
Code Review
This pull request introduces shell completion support for various shells by adding a completion command and a hidden __complete command for dynamic completion generation. The implementation uses a bridge script approach, which is a solid pattern. The code is well-structured, with the core logic encapsulated in a new completion module. My main feedback concerns the argument parsing logic within the dynamic completer, which does not correctly handle combined short flags (e.g., -ab), a standard CLI feature. This can lead to incorrect completions. I've left a specific comment with details on this issue.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces shell completion support for various shells, which is a great enhancement for the CLI's usability. My review identified two critical compilation errors in src/main.rs related to incorrect value ownership of the args vector, which will prevent the code from building. Additionally, the new tests for the completion logic in src/completion.rs are currently ineffective as they don't assert on the output, failing to verify the correctness of the generated completions. Addressing these points will ensure the new feature is robust and correct.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces shell completion support for various shells, which is a great feature for improving the CLI's usability. The implementation uses a bridge approach with a hidden __complete command to provide dynamic completions. While the overall approach is solid, I've found a critical issue in the completion logic that prevents it from correctly providing suggestions for flag values. The provided feedback includes a fix for this issue to ensure the new feature works as expected.
This PR adds shell completion support for major shells using a bridge approach with a hidden __complete command. This enables dynamic completion for subcommands and flags based on the Discovery Document for each service. Fixes googleworkspace#323
e664395 to
150f5f2
Compare
|
I have addressed all the feedback from the reviews.
All tests and clippy checks are passing. |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces shell completion support for various shells using a bridge script that calls a hidden __complete command. This is a solid approach for providing dynamic completions based on the API discovery documents. The implementation includes completion logic for subcommands, flags, and flag values. I've found one critical bug in the completion logic where it fails to provide value suggestions for a flag when it's the last argument on the command line. The fix is straightforward and I've provided a suggestion.
…lap_complete dependency
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces shell completion support for bash, zsh, fish, PowerShell, and elvish. The implementation uses a hidden __complete command to dynamically generate completions. The overall approach is solid, but the PowerShell completion script is fundamentally flawed and will not work correctly for commands with quoted arguments containing spaces. This has been flagged as a critical issue that needs to be addressed for the feature to be considered functional on PowerShell.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces shell completion support for various shells, which is a significant usability improvement. The implementation uses a bridge command __complete to handle dynamic completions, a robust approach for a CLI that interacts with multiple APIs. The code is well-structured and includes tests for the new completion logic. I've identified one high-severity issue in the generated bash completion script that could lead to incorrect behavior for completion items containing spaces. The fix is provided as a code suggestion.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces shell completion support for bash, zsh, fish, powershell, and elvish. It uses a hidden __complete command to dynamically generate completions for both static and service-based subcommands and flags. The implementation is comprehensive and includes new tests. My review found one area for improvement in the completion logic to remove some unreachable code, which will improve maintainability.
…lter duplicate flags
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces shell completion support for various shells, which is a great feature for improving the CLI's usability. The implementation uses a bridge approach with a hidden __complete command to provide dynamic completions for both static commands and dynamic, service-based commands. The core logic for parsing arguments and generating completions is comprehensive, and the addition of tests is appreciated.
Description
This PR adds shell completion support for major shells (bash, zsh, fish, powershell, elvish) using a bridge approach with a hidden
__completecommand. This enables dynamic completion for subcommands and flags based on the Discovery Document for each service.Fixes #323
Dry Run Output:
{ "method": "get", "params": { "fileId": "123" }, "scopes": ["https://www.googleapis.com/auth/drive.readonly"] }Checklist:
AGENTS.mdguidelines (no generatedgoogle-*crates).cargo fmt --allto format the code perfectly.cargo clippy -- -D warningsand resolved all warnings.pnpx changeset) to document my changes.