-
Notifications
You must be signed in to change notification settings - Fork 1
feat: shell completion support & merge install into init #22
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
- Add `codes completion [bash|zsh|fish|powershell]` command using Cobra's built-in completion generators - Add dynamic completion for config names, project names, and static values on relevant subcommands - Merge `install` command into `init`: now `codes init` handles binary self-install, shell completion setup, and environment health checks - Add install.sh for quick curl-pipe installation - Update README and Makefile to reflect the changes
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, 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 have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 WalkthroughWalkthroughThe changes refactor the CLI installation flow from an "install" command to a unified "init" command that handles binary installation, shell completion setup, and environment verification. Shell completion support for bash, zsh, fish, and PowerShell is introduced with dynamic completions for config and project names across various commands. Documentation, build configuration, and command registration are updated accordingly. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@internal/commands/commands.go`:
- Around line 601-615: RunInit doesn't check the success of installBinary() and
installShellCompletion(), so failures aren't reflected in the allGood flag;
update RunInit to capture the return values from installBinary and
installShellCompletion (the functions that currently return ("", bool)) and set
allGood = allGood && ok (or set to false on failure) and surface any error
output as needed so the final "All checks passed!" reflects actual install
success. Reference: RunInit, installBinary, installShellCompletion, and the
allGood variable.
- Around line 532-575: The installShellCompletion function currently ignores the
error from os.UserHomeDir(), which can leave homeDir empty and cause writes to
root; change the call to capture the error (homeDir, err := os.UserHomeDir()),
check err and bail out with a ui.ShowWarning or ui.ShowError before proceeding,
returning early to avoid constructing paths with an empty homeDir; update uses
in branches (zsh/bash/fish) to rely on the validated homeDir and keep existing
calls like appendCompletionLine, os.MkdirAll, os.WriteFile unchanged.
- Around line 469-530: The installBinary function currently ignores errors from
os.UserHomeDir() (calls near UserHomeDir usage for "windows" and default
fallback) which can produce invalid target paths; update both os.UserHomeDir()
calls to check the returned error and handle it by calling ui.ShowError with a
clear message and returning "", false (or otherwise aborting installation)
instead of proceeding with an empty homeDir; ensure you reference the same
symbols (installBinary, targetDir, installPath) and place the error checks
immediately after each os.UserHomeDir() call so the function never constructs or
writes to paths derived from an empty home directory.
In `@README.md`:
- Around line 127-135: The fenced code block containing the
installation/checklist output (the block that begins with triple backticks and
lines like "✓ Binary installed to /usr/local/bin/codes" and "✓ Default
configuration is working") is missing a language specifier; update that block to
use a language identifier such as text (e.g., change the opening backticks to
```text) so markdownlint MD040 is satisfied and the snippet is explicitly marked
as plain text.
🧹 Nitpick comments (2)
internal/commands/commands.go (1)
512-516: Consider streaming the binary copy instead ofReadFile.
os.ReadFileloads the entire binary into memory. Go binaries can be 20–50+ MB. Usingio.Copybetween file handles would be more memory-efficient.♻️ Suggested refactor
- sourceData, err := os.ReadFile(executablePath) - if err != nil { - ui.ShowError("Failed to read executable", err) - return "", false - } - - if err := os.WriteFile(installPath, sourceData, 0755); err != nil { - ui.ShowError("Failed to write to target location", err) - return "", false - } + src, err := os.Open(executablePath) + if err != nil { + ui.ShowError("Failed to read executable", err) + return "", false + } + defer src.Close() + + dst, err := os.OpenFile(installPath, os.O_CREATE|os.O_WRONLY|os.O_TRUNC, 0755) + if err != nil { + ui.ShowError("Failed to write to target location", err) + return "", false + } + defer dst.Close() + + if _, err := io.Copy(dst, src); err != nil { + ui.ShowError("Failed to copy executable", err) + return "", false + }internal/commands/cobra.go (1)
336-347:completeProjectNamesreturnsShellCompDirectiveDefault, which enables file completion alongside project names.This is appropriate for
StartCmd(which accepts both project names and file paths), but forProjectRemoveCmd(Line 255), file path suggestions are misleading since it only accepts project names.Consider either splitting into two functions or having
ProjectRemoveCmduse its own completion that returnsShellCompDirectiveNoFileComp.♻️ Suggested approach
// completeProjectNames provides dynamic completion for project names -func completeProjectNames(cmd *cobra.Command, args []string, toComplete string) ([]string, cobra.ShellCompDirective) { +func completeProjectNames(cmd *cobra.Command, args []string, toComplete string) ([]string, cobra.ShellCompDirective) { + projects, err := config.ListProjects() + if err != nil { + return nil, cobra.ShellCompDirectiveNoFileComp + } + var names []string + for name := range projects { + names = append(names, name) + } + // Allow file completion for commands that accept paths (e.g., start) + // but not for commands that only accept project names (e.g., project remove) + if cmd.Name() == "remove" { + return names, cobra.ShellCompDirectiveNoFileComp + } + return names, cobra.ShellCompDirectiveDefault +}
- Check os.UserHomeDir() errors in installBinary/installShellCompletion - Use io.Copy for streaming binary copy instead of ReadFile - Reflect install failures in RunInit allGood flag - Fix ProjectRemoveCmd to use NoFileComp directive - Add install.ps1 for Windows one-liner install - Add PowerShell quick install to README
Summary
Closes #12
codes completion [bash|zsh|fish|powershell]command with Cobra's built-in generators. Dynamic completion for config names (test), project names (start,project remove), and static values (defaultbehavior set,skippermissions set,config set/get).install→init:codes initnow handles binary self-install to PATH + automatic shell completion setup + environment health checks. The standaloneinstallcommand is removed.curl -fsSL .../install.sh | shfor one-liner setup.Changes
internal/commands/cobra.goCompletionCmd+completeConfigNames/completeProjectNames+ValidArgsFunctionon 6 commands, removedInstallCmdinternal/commands/commands.goinstallBinary()+installShellCompletion()+appendCompletionLine()helpers, merged intoRunInit(), removedRunInstall()cmd/codes/main.goCompletionCmd, removeInstallCmdMakefiletestandinstalltargetsinstall.shREADME.mdTest plan
go build ./...passescodes completion bash/zsh/fish/powershellall generate scripts successfullycodes __complete completion ""returnsbash zsh fish powershellcodes __complete defaultbehavior set ""returnscurrent last homecodes __complete skippermissions set ""returnstrue falsecodes installreturns "unknown command" (removed)source <(codes completion zsh)then TAB completion workscodes initinstalls binary + sets up completion + runs health checkSummary by CodeRabbit
New Features
codes initcommand that automatically configures shell completion and performs environment checksDocumentation