Skip to content
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

Zsh completion V2 #1070

Merged
merged 1 commit into from
Jun 29, 2020
Merged

Conversation

marckhouzam
Copy link
Collaborator

@marckhouzam marckhouzam commented Mar 30, 2020

This is a new implementation of Zsh completion but based on the Custom Go Completions of PRs #1035 and #1048.

The current Zsh completion has a major limitation in the fact that it does not support custom completions. Furthermore, if it ever did, it would require the program using Cobra to implement a second, zsh-flavoured version of its bash completion code.

This v2 version allows to automatically re-use custom completions once they have been migrated to the Custom Go Completion solution of PR #1035.

Advantages:

  • support custom completions (in Go only) (as mentioned above)
  • proper support for using = for flags
  • fixed size completion script of around 100 lines, so very fast on shell startup
  • allows disabling of descriptions (to feel like bash)
  • when there are no other completions, provides file completion automatically. This can be turned off on a per command basis

@jharshman jharshman added area/shell-completion All shell completions kind/feature A feature request for cobra; new or enhanced behavior labels Mar 30, 2020
@mike-stewart
Copy link

Is this going to be merged? I'm very interested in being able to apply the changes from PR 1035 for zsh completion.

@mike-stewart
Copy link

@marckhouzam I tried your last commit rebased onto master, with one modification to allow it to build (53b3354). The custom completions work great, but all other completions are broken - not even completing the commands themselves. Is that expected or a bug?

Let me know if I can contribute in any way - I'm new to Go but could take a stab at it if you can point me in the right direction.

@marckhouzam
Copy link
Collaborator Author

Thanks @mike-stewart for trying it out.
This PR is dependant on #1048. That's why completions don't work for you.

I'm about to post an updated version that will work.

@marckhouzam marckhouzam force-pushed the feat/zshCompletionV2 branch from b6e476d to 1f61673 Compare April 5, 2020 22:31
@marckhouzam
Copy link
Collaborator Author

Ok, so this new version should work.
Here is a small test program I use:

package main

import (
	"fmt"
	"os"

	"github.com/spf13/cobra"
)

var rootCmd = &cobra.Command{
	ValidArgs: []string{"replica\tanother", "rs\tdescription for rs", "ds"},
	Args:      cobra.ExactArgs(1),
	Use:       "testprog1",
	Run: func(cmd *cobra.Command, args []string) {
		fmt.Println("rootCmd called")
	},
}

var childCmd = &cobra.Command{
	Use:   "pod",
	Short: "Start the pod",
	ValidArgsFunction: func(cmd *cobra.Command, args []string, toComplete string) ([]string, cobra.BashCompDirective) {
		return []string{"these", "are\tverb", "custom\tadjective", "completions\tnoun, plural"}, cobra.BashCompDirectiveDefault
	},
	Run: func(cmd *cobra.Command, args []string) {
		fmt.Println("child called")
	},
}

var childCmd2 = &cobra.Command{
	Use:   "svc",
	Short: "Launch the service",
	ValidArgsFunction: func(cmd *cobra.Command, args []string, toComplete string) ([]string, cobra.BashCompDirective) {
		return []string{"second", "level"}, cobra.BashCompDirectiveDefault
	},
	Run: func(cmd *cobra.Command, args []string) {
		fmt.Println("child called")
	},
}

var completionCmd = &cobra.Command{
	Use:                   "completion [bash|zsh1|zsh2|fish] [--no-descriptions]",
	Short:                 "Generate completion script",
	DisableFlagsInUseLine: true,
	ValidArgs:             []string{"bash", "zsh1\tOriginal zsh completion", "zsh2\tV2 zsh completion", "fish"},
	Args:                  cobra.ExactValidArgs(1),
	Run: func(cmd *cobra.Command, args []string) {
		switch args[0] {
		case "bash":
			cmd.Root().GenBashCompletion(os.Stdout)
			break
		case "zsh1":
			cmd.Root().GenZshCompletion(os.Stdout)
			break
		case "zsh2":
			cmd.Root().GenZshCompletionV2(os.Stdout, !completionNoDesc)
			break
		case "fish":
			cmd.Root().GenFishCompletion(os.Stdout, !completionNoDesc)
			break
		}
	},
}

var (
	completionNoDesc bool
	testFlag         string
)

func main() {
	completionCmd.PersistentFlags().BoolVar(
		&completionNoDesc,
		"no-descriptions", false,
		"disable completion description for shells that support it")
	completionCmd.PersistentFlags().StringVar(
		&testFlag,
		"test-flag", "",
		"just to test")
	rootCmd.AddCommand(childCmd)
	rootCmd.AddCommand(completionCmd)
	childCmd.AddCommand(childCmd2)
	rootCmd.Execute()
}

@mike-stewart
Copy link

@marckhouzam This works great on my (fairly simple) app w/ custom completions.

when there are no other completions, provides file completion automatically. This can be turned off on a per command basis

What is the correct way to turn off this file completion?

@marckhouzam
Copy link
Collaborator Author

I'll direct you to the documentation so you can let us know if it's understandable or not.

Look for the description for BashCompDirective in https://github.com/spf13/cobra/blob/master/bash_completions.md#1-custom-completions-of-nouns-written-in-go

@mike-stewart
Copy link

@marckhouzam Yes, that works. I had implemented that already for some commands, but thought your comment suggested a way to disable file completion for commands where custom completion is not being used.

I worked around that by adding this to commands that I don't want any completion on:

ValidArgsFunction: func(cmd *cobra.Command, args []string, toComplete string) ([]string, cobra.BashCompDirective) {
	return nil, cobra.BashCompDirectiveNoFileComp
},

Thanks again for getting this branch working.

@marckhouzam
Copy link
Collaborator Author

I worked around that by adding this to commands that I don't want any completion on:

ValidArgsFunction: func(cmd *cobra.Command, args []string, toComplete string) ([]string, cobra.BashCompDirective) {
	return nil, cobra.BashCompDirectiveNoFileComp
},

That's exactly how you should do it. Should we add an example in the docs for this case, which I expect will be common or will users figure it out easily enough? As the first user, what do you think?

@mike-stewart
Copy link

mike-stewart commented Apr 6, 2020

@marckhouzam The workaround was obvious upon reading the code and documentation, but it might not have been immediately apparent to a casual reader unless they specifically set out to disable file completion. Adding it to the documentation would make it clearer that it's a "feature" that is available to users.

@marckhouzam
Copy link
Collaborator Author

I will rebase this later today to adapt to the BashCompDirective to ShellCompDirective change of #1082 .

@marckhouzam
Copy link
Collaborator Author

marckhouzam commented Apr 7, 2020

Adding it to the documentation would make it clearer that it's a "feature" that is available to users.

@mike-stewart would you like to make a small PR to add this explanation to the documentation, as it applies to all shells?

@marckhouzam marckhouzam force-pushed the feat/zshCompletionV2 branch from 1f61673 to 657d1c4 Compare April 7, 2020 03:12
@marckhouzam
Copy link
Collaborator Author

I rebased this PR after the BashCompDirective to ShellCompDirective change.

@itskingori
Copy link

This PR is not totally finished, but before spending more time on it, I wanted to know if there is some interest for such a change?

@marckhouzam There's definitely interest in this change! 😅 I've been following your other PRs and was glad when they got merged and v1.0.0 released. The only thing missing is this PR.

For context I've used custom Go completions to implement completion of flag values using RegisterFlagCompletionFunc but if I understand correctly, that would work for Bash and Fish only at the moment.

@marckhouzam
Copy link
Collaborator Author

I will soon get back to this PR. But first I want migrate the Helm project to using Cobra 1.0.0 🎉

@marckhouzam
Copy link
Collaborator Author

marckhouzam commented Apr 13, 2020

I looked a bit into the existing zsh completion support to see if we really need a V2 version or if we could completely fulfill the current zsh completion API with this new solution. IIUC the current zsh solution has some APIs of its own that are not relevant to bash completion. This seems confirmed by these comments from the original author of the current zsh completion solution:
#646 (comment) and #646 (comment)

As that comment mentions, there really should not be different APIs for different shells but we should strive for a single API working for all shells.

I think we can get very close to that goal with the current PR. But that confirms we do need to introduce a V2 version for zsh completion as this PR should not support the zsh-only API of the other zsh solution.

Now that I'm more comfortable about having to introduce a V2, I'll be able to get a polished version ready soon.

@marckhouzam marckhouzam force-pushed the feat/zshCompletionV2 branch 2 times, most recently from 93e8ceb to 37bd6e8 Compare April 15, 2020 02:56
@marckhouzam marckhouzam changed the title RFC: Zsh completion V2 (WIP) WIP: Zsh completion V2 Apr 15, 2020
@marckhouzam
Copy link
Collaborator Author

I've rebased on master and updated the documentation. To my knowledge, all types of completions are supported except the ones enabled by BashCompFilenameExt (filtering by file extension), BashCompOneRequiredFlag (required flags) and BashCompSubdirsInDir (filtering by directory).

I'm not removing the WIP label because I'm still thinking about how to avoid having a V2 version.

The only two APIs that are zsh-specific are MarkZshCompPositionalArgumentWords
and MarkZshCompPositionalArgumentFile . I believe the first one corresponds to ValidArgs so would be easy to handle. IIUC, the second one does not have a corresponding one in Bash-completion, but maybe we should add one to standardize, and then be backwards-compatible for zsh.

I'll keep thinking about it.

fi
}

compdef _%[1]s %[1]s
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line allows to source the zsh completion script as is currently done for helm and kubectl. I like the idea of providing that option.

However, I need to point out that @jharshman disagreed with this in #887 (comment), so he may prefer to have this removed, once we get to review time.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@marckhouzam yeah I still maintain my previous opinion about this.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jharshman is right. I found this article to be a good explanation of why sourcing a completion script should not be recommended: https://medium.com/@jzelinskie/please-dont-ship-binaries-with-shell-completion-as-commands-a8b1bcb8a0d0

For projects that need to keep backwards-compatibility, they can insert that one extra line themselves. I will need to do that for Helm.

I will remove this line in my next update.

@marckhouzam
Copy link
Collaborator Author

marckhouzam commented Apr 18, 2020

I've continued investigating the existing zsh-completion support.
As explained by the original author (#646 (comment)), the current zsh support was designed independently of bash completion and is not well aligned to it.

There are multiple differences but one major difference is that the current zsh completion does not do file completion by default while bash does (see #886). Say I have a program testprog that has no sub-commands and no ValidArgs then ./testprog [tab][tab] will list all files for bash, but will not for zsh.

To get file completion (for the first argument only) for zsh, the program developer would need to add the line

command.MarkZshCompPositionalArgumentFile(1)

On the other hand, to turn off file completion for bash, the developer would need to use the new ShellCompDirectiveNoFileComp with ValidArgsFunction.

This means that a developer that wants to support both bash and zsh must write code to handle zsh completion (current version) and different code for the bash completion.

I think we should strive to align completion behavior across all shells or else it becomes a nightmare for the program developers.

This leads me to backwards-compatibility. I strongly feel that the new zsh completion of this PR should provide file completion by default like bash, as justified above. This would technically be a breaking-change compared to the current zsh completion script.

So I think we are at the point of making a decision: do we do a V2 version for zsh like this PR is doing right now, or do we modify the current behavior of zsh completion to align with bash?

@jharshman I'm looking for guidance from the maintainers, as this PR is close to finished and mostly need to know which direction to take with respect to V2 or not. Thanks in advance.

@jharshman
Copy link
Collaborator

@marckhouzam Having all the shell completion work the same is definitely appealing.
@wfernandes @jpmcb what do you guys think?

@jpmcb
Copy link
Collaborator

jpmcb commented Apr 20, 2020

I think we should strive to align completion behavior across all shells or else it becomes a nightmare for the program developers.

I agree. Otherwise, I think we end up with "drift" between the different shells which creates pain for the cobra application developers. They end up having to maintain any number of different completions that all have different APIs for similar behavior.

I'm in favor of introducing breaking changes for zsh completions that strives for closer parity among the various shell completions. And maybe we begin to deprecate the old version or announce that this is the new way to go about completions. But I'm not sure how this project in the past has approached deprecating features so I'll leave that up to @jharshman

@jharshman jharshman merged commit 2c5a0d3 into spf13:master Jun 29, 2020
@marckhouzam marckhouzam deleted the feat/zshCompletionV2 branch June 29, 2020 20:12
@marckhouzam
Copy link
Collaborator Author

Thank you @jpmcb and @jharshman for your help on getting this done!

I will rebase #1122 now.

@jakrosh
Copy link

jakrosh commented Jul 15, 2020

@marckhouzam I am having trouble getting custom dynamic flag completions for zsh working for my project. The custom flag completions work in bash, but when I hit tab tab after typing in the flag, nothing happens. I went through the code in custom_completions.go, bash_completions.go, and zsh_completions.go, and noticed that the var flagCompletionFunctions(which stores a map of the custom flag completion functions) is used in bash_completions.go, but not zsh_completions.go. Is that why I can't get custom flag completions for zsh working, or is something else the problem?

@marckhouzam
Copy link
Collaborator Author

@jakrosh could you give an example to reproduce the problem?

Also, how did you setup the zsh completion script in your environment?

@marckhouzam
Copy link
Collaborator Author

@jakrosh actually, the first step to debug your custom completions is to call the __complete command manually. So let's say the following doesn't work:

program --flag <TAB>

Then you should run

program __complete --flag "" <ENTER>

to see if you get the proper completions.

@jakrosh
Copy link

jakrosh commented Jul 15, 2020

running
program __complete --flag "" <ENTER>
gives my custom completions correctly. however, running
program __complete --flag jen<ENTER>
does not give only the flag that starts with "jen"
Screen Shot 2020-07-15 at 11 53 12 AM

I set up the zsh completion script with this line:
dctl completion zsh > "$ZSH/completions/_dctl"

@marckhouzam
Copy link
Collaborator Author

program __complete --flag jen<ENTER>
does not give only the flag that starts with "jen"

This is because the function you registered with RegisterFlagCompletionFunc() does not filter based on the toComplete filter. But that should not be a problem as zsh filters things itself (at least for me).
If you want to filter yourself, you just need to add something like if strings.HasPrefix(completion, toComplete) { in your function in RegisterFlagCompletionFunc()

What exactly is the problem you are seeing with zsh compared to bash?

I set up the zsh completion script with this line:
dctl completion zsh > "$ZSH/completions/_dctl"

That should be ok.

@jakrosh
Copy link

jakrosh commented Jul 15, 2020

Sorry for being unclear, but my problem is when I type
dctl -a <TAB><TAB>
in bash, the custom completion list shows up:
Screen Shot 2020-07-15 at 1 06 43 PM

but in zsh nothing happens:
Screen Shot 2020-07-15 at 1 16 25 PM

As you saw in my previous comment, when I use __complete the custom completion list does show up in zsh. I'm confused why it works in debug mode, but not in regular.

@marckhouzam
Copy link
Collaborator Author

It may be your zsh that is not configured to list all the possible completions where there are more than one.
If you try ls <TAB> do you see all the choices of files listed for you?

@jakrosh
Copy link

jakrosh commented Jul 15, 2020

yes, I can see the files listed:
Screen Shot 2020-07-15 at 1 43 43 PM

@marckhouzam
Copy link
Collaborator Author

Then you can try to see what the zsh completion script generated by Cobra does. For example:

export BASH_COMP_DEBUG_FILE /tmp/zshdebug
dctl -a <TAB>
cat /tmp/zshdebug

You'll see a bunch of traces following what the script does.

@austintraver
Copy link

Thank you for all that hard work @marckhouzam

I'm really excited for this feature, @jharshman how far out are we from the next release?

@brianpursley
Copy link
Contributor

+1 for @austintraver's question, when will this be available in a release?

@marckhouzam
Copy link
Collaborator Author

1.1.0 has just been released!
Thanks @jharshman.

For people that will want to start using zsh completion from Cobra be aware that custom completions will only work for zsh if they are implemented in Go (using ValidArgsFunction and RegisterFlagCompletionFunc()). They will not work for the ones using bash scripts injected through BashCompletionFunction.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/shell-completion All shell completions kind/feature A feature request for cobra; new or enhanced behavior
Projects
None yet
Development

Successfully merging this pull request may close these issues.