Make sure we quote brackets when generating zsh completion

[chmouel]

zsh completion would fail when we have brackets in description, for example :

% source   <(./tkn completion zsh)
% tkn task list[TAB]
_arguments:comparguments:319: invalid option definition: --template[Template string or path to template file to use when -o=go-template, -o=go-template-file. The template format is golang templates [http://golang.org/pkg/text/template/#pkg-overview].]:filename:_files

[CLAassistant]

All committers have signed the CLA.

[chmouel]

CLA link doesnt seem to work 😢

[chmouel]

I am not really sure why unittest fails, the spellchecking is only done on bash_completion

[rsteube]

The shellcheck simply changed: #889 - problably an update or similar.
Duplicate of #885, this one's got a test though which is good.

[JohnnyVim]

#889 has been merged into master. A rebase will fix the tests.

[chmouel]

thanks for the heads up i rebased!

[umarcor]

@chmouel, would you mind merging #885 into this PR?

[chmouel]

@umarcor sure! Done.

[chmouel]

I guess that's ready for merge now, isntit ?

[rhuss]

Can we get this merged, please ? The changes look reasonable simple and valid (and it also bites us for our ZSH completion business).

Thanks !

[navidshaikh]

Thanks Chmouel!

Can we get this merged please?

It should help us vendor cobra from master branch commit after merge (since the next cobra milestone is Dec 20).

[corneliusweig]

@chmouel Can you please include the following patch in your PR to fix #989 :

diff --git zsh_completions.go zsh_completions.go
index b58a5d6..e2476b6 100644
--- zsh_completions.go
+++ zsh_completions.go
@@ -25,6 +25,7 @@ var (
 		"extractFlags":                zshCompExtractFlag,
 		"genFlagEntryForZshArguments": zshCompGenFlagEntryForArguments,
 		"extractArgsCompletions":      zshCompExtractArgumentCompletionHintsForRendering,
+		"escapeText":                  zshCompQuoteFlagDescription,
 	}
 	zshCompletionText = `
 {{/* should accept Command (that contains subcommands) as parameter */}}
@@ -41,7 +42,7 @@ function {{$cmdPath}} {
   case $state in
   cmnds)
     commands=({{range .Commands}}{{if not .Hidden}}
-      "{{.Name}}:{{.Short}}"{{end}}{{end}}
+      "{{.Name}}:{{.Short | escapeText}}"{{end}}{{end}}
     )
     _describe "command" commands
     ;;
@@ -334,5 +335,7 @@ func zshCompFlagCouldBeSpecifiedMoreThenOnce(f *pflag.Flag) bool {
 func zshCompQuoteFlagDescription(s string) string {
 	return strings.NewReplacer("'", `'\''`,
 		"[", `\[`,
-		"]", `\]`).Replace(s)
+		"]", `\]`,
+		"$(", `\$(`,
+	).Replace(s)
 }
diff --git zsh_completions_test.go zsh_completions_test.go
index 8079f6c..a9405fe 100644
--- zsh_completions_test.go
+++ zsh_completions_test.go
@@ -240,6 +240,25 @@ func TestGenZshCompletion(t *testing.T) {
 				`--ptest\[ptest]:filename:_files -g "-\(/\)"`,
 			},
 		},
+		{
+			name: "escape text in subcommand description",
+			root: func() *Command {
+				r := &Command{
+					Use:  "rootcmd",
+					Long: "Long rootcmd description",
+				}
+				d := &Command{
+					Use:   "subcmd1",
+					Short: "$(echo foo)",
+					Run:   emptyRun,
+				}
+				r.AddCommand(d)
+				return r
+			}(),
+			expectedExpressions: []string{
+				`\$\(echo foo\)`,
+			},
+		},
 	}
 
 	for _, tc := range tcs {

[chmouel]

Hello @corneliusweig, I could surely do but what is the point if maints does not seem to want to merge this ?

[corneliusweig]

@chmouel I think a new release is due in December and PRs are usually merged shortly before that. But clearly, the maintainers have other responsibilities so lets bundle these improvements in a single PR.

[chmouel]

@corneliusweig sure, let's hope this gets merged, I have updated the PR, thanks

[github-actions]

This PR is being marked as stale due to a long period of inactivity

[rhuss]

I don't think this PR should be considered stale.

[umarcor]

@rhuss >70% of the open issues are already labeled as 'kind/stale' and it seems not possible to tell the bot otherwise. I believe it is a misconfiguration.

[corneliusweig]

@umarcor If the bot doesn't obey, just do a git commit --amend and force-push the result. Then the PR won't be stale anymore :)

[umarcor]

@corneliusweig, are you sure about that? That's what I would expect from any regular bot that handles stale issues. Some do also detect edition of comments, so it is not even required to force-push (any participant can help). However, the bot that is being used in this repo seems not to unmark anything: https://github.com/actions/stale. Unfortunately, most of the actions in https://github.com/actions feel like placeholders for marketing purposes. Some months will be required until quality catches up. See #952 (comment).

[corneliusweig]

No, I'm not sure. I was merely guessing. But have you tried? I'm curious what will happen 😄

[umarcor]

No, I'm not sure. I was merely guessing. But have you tried? I'm curious what will happen 😄

I preventively ensured that all the PRs I proposed are updated and have a recent comment: https://github.com/spf13/cobra/pulls?q=is%3Apr+is%3Aopen+sort%3Aupdated-desc+author%3Aumarcor 😄

[chmouel]

I gave up updating this PR, afer eight months being hopeful the maintainers just don't seem to care about those fixes

[marckhouzam]

The zsh completion logic has been revamped by #1070 and this problem has been addressed.

@jpmcb I believe we can close this one now that #1070 has been merged.

[navidshaikh]

@marckhouzam : Thanks for the fix!

Noticed a difference in the way auto-completion of flags starting with - or -- are handled.

With the patch in this PR #899 , it also allows to auto-complete the flags, for eg:

➜  client git:(master) kn service create [tab]

would add - after create and another [tab] after - lists all the flags

kn service create -[tab]
zsh: do you wish to see all 129 possibilities (43 lines)? [tab]
--annotation               -a  -- Service annotation to set. name=value; you may provide this flag any number of times to set multiple annotations. To unset, specify the annotation name fo
--arg                          -- Add argument to the container command. Example: --arg myArg1 --arg --myArg2 --arg myArg3=3. You can use this flag multiple times.                         
[..]

While if I try using #1070, and [tab] after kn service create it appends :0 to the command

kn service create [tab]

gives

kn service create :0

while if I type - after kn service create, it would list the available flags.

So the difference is, this patch also takes care of auto-completing the flags, while #1070 would add :0.

Reproduce:

1. With the patch in this PR

git clone https://github.com/knative/client && cd client
./hack/build.sh 
source <(./kn completion zsh)
compdef _kn kn

2. With Zsh completion V2 #1070

git clone -b pr/cobra-upgrade https://github.com/navidshaikh/client && cd client
./hack/build.sh 
source <(./kn completion zsh)
compdef _kn kn

[marckhouzam]

@navidshaikh Thank you for trying the new zsh completion early, it will help confirm there is no issues.

Noticed a difference in the way auto-completion of flags starting with - or -- are handled.
With the patch in this PR #899 , it also allows to auto-complete the flags, for eg:

➜  client git:(master) kn service create [tab]

would add - after create and another [tab] after - lists all the flags

This change is on purpose to align it with bash-completion behaviour. It is important that shell-completion behaves the same across all shells, so we had to make this change since bash completion only completes flags if the user types the first -. I also believe this is the better behaviour.

I realize now that I didn't document it in: https://github.com/spf13/cobra/blob/master/zsh_completions.md
Do you want to post a PR to add this information?

While if I try using #1070, and [tab] after kn service create it appends :0 to the command

kn service create [tab]

gives

kn service create :0

This is not right.
But thanks to the instructions you gave on how to reproduce I was able to debug this and it looks like it is a particularity of kn.
First, to help see the problem you can do:

$ ./kn __complete s<ENTER>
:0
Completion ended with directive: ShellCompDirectiveDefault
service Manage Knative services
source  Manage event sources
:4
Completion ended with directive: ShellCompDirectiveNoFileComp

You'll notice that two ShellCompDirective are printed. This should not happen. This is where the :0 comes from, as the zsh completion script does not expect to have two directives. In fact if you complete

$ ./kn <tab>
broker      -- Manage message broker
completion  -- Output shell completion code
help        -- Help about any command
options     -- Print the list of flags inherited by all commands
plugin      -- Manage kn plugins
revision    -- Manage service revisions
route       -- List and describe service routes
service     -- Manage Knative services
source      -- Manage event sources
trigger     -- Manage event triggers
version     -- Show the version of this client
:0

you will notice the extra :0 at the end which should not be there.

From what I can see, kn executes an extra command at the start:
https://github.com/knative/client/blob/217a8133446acfadd19600a768b2be52fb7afd19/cmd/kn/main.go#L119
https://github.com/knative/client/blob/217a8133446acfadd19600a768b2be52fb7afd19/cmd/kn/main.go#L144

I believe this is what is causing the extra ShellCompDirective to be printed when the __complete command is called.
I didn't spend more time to try to understand why kn runs such a command, so I'm not sure if it is something to fix in kn or if we should put something in Cobra to protect against such a case.

Could you give more information about that?

[marckhouzam]

This change is on purpose to align it with bash-completion behaviour. It is important that shell-completion behaves the same across all shells, so we had to make this change since bash completion only completes flags if the user types the first -. I also believe this is the better behaviour.

I realize now that I didn't document it in: https://github.com/spf13/cobra/blob/master/zsh_completions.md
Do you want to post a PR to add this information?

I had to make a documentation PR for something else, so I did this also. See #1169.

[navidshaikh]

I believe this is what is causing the extra ShellCompDirective to be printed when the __complete command is called.
I didn't spend more time to try to understand why kn runs such a command, so I'm not sure if it is something to fix in kn or if we should put something in Cobra to protect against such a case.

Could you give more information about that?

@marckhouzam : thank you for debugging this!
kn offers plugins support. We need to find the exact set of args (valid/invalid) without flags (and their values).
The computed set of args is then used to check if there is matching kn-plugin available which implements the given command.

For example:
Given a plugin kn-source-kafka exists (in the path kn looks for) and provided command is
kn --namespace foo source kafka create -h ;
it would compute the args as [kn source kafka create] and then combine the args to find the matching plugins (longest path first). If found the plugin among combination of these paths, kn will execute the plugin.
This happens before we execute the actual kn root command, and in order to extract the args without the flags (and their values), we inject an extractCommand as you observed and execute it.
This way the extractCommand leverages cobra parsing logic for flags, etc and returns the list of args we are looking for.

[rhuss]

Of course, there might be a better way to let cobra parse the arguments without temporarily creating a command. If so, please let us know.

[marckhouzam]

Of course, there might be a better way to let cobra parse the arguments without temporarily creating a command. If so, please let us know.

I'm not sure if it's the best way, but this seems to strip all the flags and their values and leaves you with args only:

	rootCmd.FParseErrWhitelist = cobra.FParseErrWhitelist{UnknownFlags: true}
	if err := rootCmd.ParseFlags(os.Args[1:]); err != nil {
		fmt.Printf("Error while parsing flags from args %v: %s\n", os.Args[1:], err.Error())
	}
	finalArgs := rootCmd.Flags().Args()
	fmt.Printf("args %v\n", finalArgs)

[rsteube]

Yeah, that's probably the right way - i used Traverse / ParseFlags as well: https://github.com/rsteube/carapace/blob/master/carapace.go#L194

[navidshaikh]

Thanks @marckhouzam @rsteube for the help! we could remove the extra command execution and use inbuilt parsing utils, and the extra directive / :0 issue seems to be resolved.

This change is on purpose to align it with bash-completion behaviour. It is important that shell-completion behaves the same across all shells, so we had to make this change since bash completion only completes flags if the user types the first -. I also believe this is the better behaviour.

Is it shell-completion limitation to have - typed for auto-completing the flags ?
IMO, different shells bring distinct capabilities which could be leveraged to provide users with more power.

[rsteube]

No, as far as i know that depends how you configure it. I think cobra had something like a required option for flags, which is meant for those to be shown during completion even without preceeding -. Still, that working depends on how the completion is generated. Seems to be the common default to show flags only with preceeding - though. Personally i normally prefer it that way and intentionally generated my completion this way so far (although there's a good reason to do it the other way on some occasions).

[marckhouzam]

I believe @rsteube has explained things very well. Here is the doc for marking a flag "required": https://github.com/spf13/cobra/blob/master/shell_completions.md

[marckhouzam]

Whoops, more precise URL for required flags
https://github.com/spf13/cobra/blob/master/shell_completions.md#mark-flags-as-required