feat: add shell completions - #33
Conversation
|
Warning Review limit reached
More reviews will be available in 43 minutes and 40 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (10)
📝 WalkthroughWalkthroughThe PR adds shell completion generation to ChangesShell Completion Implementation and Distribution
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/main.rs (1)
170-235: ⚡ Quick winGenerate shell completions from the clap derive model (reduce drift)
HandwrittenBASH_COMPLETION/ZSH_COMPLETION/FISH_COMPLETIONinsrc/main.rscan desync from yourderive Parser/Argsas flags, help text, or value-enums change.In clap 4.5.x, the recommended approach is to build a
clap::Commandfrom the derived parser (CommandFactory/Opt::commandorParser::command) and feed it toclap_complete::generatefor bash/zsh/fish (as in the officialclap_completecompletion-derive.rsexample). Because the scripts are generated from clap’s current option metadata, enum/value changes stay in sync automatically whenever you regenerate (e.g., at build time or when handling your--completionrequest).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main.rs` around lines 170 - 235, The hardcoded BASH_COMPLETION/ZSH_COMPLETION/FISH_COMPLETION constants and the completion_script function will drift from your clap derive types; replace them by constructing a clap::Command from your derived parser (use Parser::command or CommandFactory::command on your Opt/Args type) and call clap_complete::generate (or generate_to) to produce the bash/zsh/fish output at runtime (e.g., when handling the --completion option) rather than returning the static strings; update completion_script to invoke the generator with the produced Command and the requested CompletionShell and return or print the generated script accordingly.tests/e2e/home-manager/completions.nix (1)
39-51: 🏗️ Heavy liftMake the zsh path assert completion candidates, not function internals.
This only proves that
_whyautoloaded and still references_path_commands; it does not verify thatwhy lactually offersls. A zsh-specific regression in the generated matches would still pass here, so the E2E coverage is weaker than the bash/fish branches.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/e2e/home-manager/completions.nix` around lines 39 - 51, The current test only checks that the autoloaded function `_why` references `_path_commands`, which verifies internals not user-visible completions; change the tail of the test so after `compinit` and autoloading `_why` it invokes zsh's completion machinery for the command prefix (e.g., simulate completing `why l`) and asserts that the candidate `ls` appears in the output instead of grepping for `_path_commands`; update the commands that currently write to "$TMPDIR/zsh.out" to capture the completion result for `why l` and check for `ls` (keeping use of `compinit`, `autoload -Uz _why`, and any existing TMPDIR output handling).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@nix/package.nix`:
- Around line 18-22: postInstall currently runs the built binary ($out/bin/why
--completion ...) which breaks cross-builds; instead generate or install
completions using a build-platform runnable tool: move the completion generation
into the buildInputs/buildPackages side (e.g., use stdenv.buildPackages or
buildPackages to call the why binary built for the build platform or use a qemu
wrapper), or ship pre-generated completion files and reference them in
postInstall; update the postInstall block that calls installShellCompletion to
read completions from those pre-generated files or from a build-side generator
rather than executing $out/bin/why at build time.
---
Nitpick comments:
In `@src/main.rs`:
- Around line 170-235: The hardcoded
BASH_COMPLETION/ZSH_COMPLETION/FISH_COMPLETION constants and the
completion_script function will drift from your clap derive types; replace them
by constructing a clap::Command from your derived parser (use Parser::command or
CommandFactory::command on your Opt/Args type) and call clap_complete::generate
(or generate_to) to produce the bash/zsh/fish output at runtime (e.g., when
handling the --completion option) rather than returning the static strings;
update completion_script to invoke the generator with the produced Command and
the requested CompletionShell and return or print the generated script
accordingly.
In `@tests/e2e/home-manager/completions.nix`:
- Around line 39-51: The current test only checks that the autoloaded function
`_why` references `_path_commands`, which verifies internals not user-visible
completions; change the tail of the test so after `compinit` and autoloading
`_why` it invokes zsh's completion machinery for the command prefix (e.g.,
simulate completing `why l`) and asserts that the candidate `ls` appears in the
output instead of grepping for `_path_commands`; update the commands that
currently write to "$TMPDIR/zsh.out" to capture the completion result for `why
l` and check for `ls` (keeping use of `compinit`, `autoload -Uz _why`, and any
existing TMPDIR output handling).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ec7bc32a-4e64-4d6e-a92b-dc6cfab7a342
⛔ Files ignored due to path filters (1)
flake.lockis excluded by!**/*.lock
📒 Files selected for processing (7)
.github/scripts/generate_formula.sh.github/workflows/ci.ymlREADME.mdflake.nixnix/package.nixsrc/main.rstests/e2e/home-manager/completions.nix
fa931d1 to
517e520
Compare
1 participant
Summary
why --completion <bash|zsh|fish>without introducing completion subcommandswhy <TAB>command-name completion to each shell's native command completion pathTesting
cargo +stable fmt --checkcargo +stable test --locked— 20 passedcargo +stable clippy --all-targets -- -D warningsnix build .#checks.x86_64-linux.home-manager-completionsnix flake check— all checks passedNotes
nix flake checkstill warns about the existinghomeManagerModulesoutput name, but the check succeeds.Summary by CodeRabbit
New Features
--completionflagDocumentation
Tests