-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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
Issue #107: add basic zsh completion (command hierarchy only) #497
Conversation
zsh_completions.go
Outdated
} | ||
|
||
func (fw *fWriter) fWriteLn(format string, a ...interface{}) (int, error) { | ||
return io.WriteString(fw, fmt.Sprintf(format+"\n", a...)) |
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.
I don't like this fWriter struct.
Why don't you use just fmt.Fprintln(w, format, a1, a2)
like in bash_completions.go? It's simpler and nicer
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.
Yeah sure, I can change to fmt.Fprintln, I don't mind.
byParent := groupByParent(commands) | ||
|
||
for p, c := range byParent { | ||
names := names(c) |
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.
variable names
defined but not used.
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.
It is used two lines below?
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.
Oh sorry, I didn't notice it.
LGTM but I know nothing about zsh syntax and how it works. |
I'm also far from being a zsh expert ;) |
LGTM I'm a zsh user that has written some zsh syntax files, but I'm far from being an expert. This PR adds basic completion (as stated in the title of this PR). I think it is a good start for having a complete zsh completion support in cobra. |
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.
Very cool! Thank you for your contribution!
Let's give it a try! 😄
This change extends the `cockroach gen autocomplete` command to support the generation of `zsh` completion files. This has been supported by `spf13/cobra` since spf13/cobra#497. Release note (cli change): The "gen autocomplete" command can now generator zsh-compatible completion files by passing "zsh" as an argument.
This change extends the `cockroach gen autocomplete` command to support the generation of `zsh` completion files. This has been supported by `spf13/cobra` since spf13/cobra#497. Release note (cli change): The "gen autocomplete" command can now generator zsh-compatible completion files by passing "zsh" as an argument.
Support for flags still missing though. Maybe we can go from here.