Conversation
There was a problem hiding this comment.
Overall this looks solid and keeps the interface simple.
Main things I called out:
- Prefer
cmd.OutOrStdout()overos.Stdoutso completion generation respects Cobra output configuration (helps with wrappers/tests). - Return an explicit error in the
switchdefault even though arg validation currently prevents it; this keeps behavior safer if validation changes. - Tweak the Linux bash install snippet to use
sudo teeso the/etc/...redirect is actually copy/paste-able.
| import ( | ||
| "os" | ||
|
|
||
| "github.com/spf13/cobra" | ||
| ) |
There was a problem hiding this comment.
Nice addition. Since the RunE below can use cmd.OutOrStdout(), you can drop the direct os.Stdout dependency and keep output redirection/testability consistent with Cobra.
| import ( | |
| "os" | |
| "github.com/spf13/cobra" | |
| ) | |
| import ( | |
| "fmt" | |
| "github.com/spf13/cobra" | |
| ) |
| RunE: func(cmd *cobra.Command, args []string) error { | ||
| switch args[0] { | ||
| case "bash": | ||
| return cmd.Root().GenBashCompletion(os.Stdout) | ||
| case "zsh": | ||
| return cmd.Root().GenZshCompletion(os.Stdout) | ||
| case "fish": | ||
| return cmd.Root().GenFishCompletion(os.Stdout, true) | ||
| } | ||
| return nil | ||
| }, |
There was a problem hiding this comment.
Consider writing to cmd.OutOrStdout() instead of os.Stdout so this respects cmd.SetOut(...) (useful for tests/wrappers). Also, even though OnlyValidArgs should prevent this, returning an explicit error on unexpected values makes the function safer if the arg validation ever changes.
| RunE: func(cmd *cobra.Command, args []string) error { | |
| switch args[0] { | |
| case "bash": | |
| return cmd.Root().GenBashCompletion(os.Stdout) | |
| case "zsh": | |
| return cmd.Root().GenZshCompletion(os.Stdout) | |
| case "fish": | |
| return cmd.Root().GenFishCompletion(os.Stdout, true) | |
| } | |
| return nil | |
| }, | |
| RunE: func(cmd *cobra.Command, args []string) error { | |
| out := cmd.OutOrStdout() | |
| switch args[0] { | |
| case "bash": | |
| return cmd.Root().GenBashCompletion(out) | |
| case "zsh": | |
| return cmd.Root().GenZshCompletion(out) | |
| case "fish": | |
| return cmd.Root().GenFishCompletion(out, true) | |
| default: | |
| return fmt.Errorf("unsupported shell: %q", args[0]) | |
| } | |
| }, |
| # Linux: | ||
| $ kernel completion bash > /etc/bash_completion.d/kernel | ||
| # macOS: | ||
| $ kernel completion bash > $(brew --prefix)/etc/bash_completion.d/kernel |
There was a problem hiding this comment.
Minor docs nit: the Linux > /etc/... redirection will fail unless the whole redirect runs as root. Swapping to sudo tee tends to make this copy/paste-able.
| # Linux: | |
| $ kernel completion bash > /etc/bash_completion.d/kernel | |
| # macOS: | |
| $ kernel completion bash > $(brew --prefix)/etc/bash_completion.d/kernel | |
| # Linux: | |
| $ kernel completion bash | sudo tee /etc/bash_completion.d/kernel > /dev/null | |
| # macOS: | |
| $ kernel completion bash > $(brew --prefix)/etc/bash_completion.d/kernel |
|
This already exists |
No description provided.