DisableFlagParsing must trigger custom completion for flag names

[marckhouzam]

Closes #1160

When a command has set DisableFlagParsing=true, it means Cobra should not be handling flags for that command but should let the command handle them itself. In fact, Cobra normally won't have been told at all about flags for this command.

Not knowing about the flags of the command also implies that Cobra cannot perform flag completion as it does not have the necessary info.

When DisableFlagParsing==true this commit makes sure ValidArgsFunction will be called even for flag name completion, which will allow the program to handle completion for its own flags in that case.

Here is a program to test this:

package main

import (
	"os"

	"github.com/spf13/cobra"
)

var completionNoDesc bool

var completionCmd = &cobra.Command{
	Use:       "completion [bash|zsh|fish|powershell]",
	Short:     "Generate completion script",
	ValidArgs: []string{"bash\tbash completion", "zsh", "fish", "powershell"},
	Args:      cobra.ExactValidArgs(1),
	Run: func(cmd *cobra.Command, args []string) {
		switch args[0] {
		case "bash":
			cmd.Root().GenBashCompletion(os.Stdout)
		case "zsh":
			if !completionNoDesc {
				cmd.Root().GenZshCompletion(os.Stdout)
			} else {
				cmd.Root().GenZshCompletionNoDesc(os.Stdout)
			}
		case "fish":
			cmd.Root().GenFishCompletion(os.Stdout, !completionNoDesc)
		case "powershell":
			cmd.Root().GenPowerShellCompletion(os.Stdout)
		}
	},
}

func emptyRun(*cobra.Command, []string) {}

func main() {
	completionCmd.Flags().BoolVar(
		&completionNoDesc,
		"no-descriptions", false,
		"disable completion description for shells that support it")

	flagValidArgs := func(cmd *cobra.Command, args []string, toComplete string) ([]string, cobra.ShellCompDirective) {
		return []string{"--flag", "-f"}, cobra.ShellCompDirectiveNoFileComp
	}

	rootCmd := &cobra.Command{Use: "testprog", Args: cobra.NoArgs, Run: emptyRun}
	rootCmd.AddCommand(completionCmd)

	child1Cmd := &cobra.Command{
		Use:                "child1",
		Run:                emptyRun,
		DisableFlagParsing: true,
		ValidArgsFunction:  flagValidArgs,
	}
	child2Cmd := &cobra.Command{
		Use:                "child2",
		Run:                emptyRun,
		DisableFlagParsing: false,
		ValidArgsFunction:  flagValidArgs,
	}
	rootCmd.AddCommand(child1Cmd, child2Cmd)

	if err := rootCmd.Execute(); err != nil {
		os.Exit(1)
	}
}

then for bash

$ source <(./testprog completion bash)
# child1 has DisableFlagParsing == true
$ ./testprog child1 -[tab][tab]
--flag  -f
# ValidArgsFunction is called and completes the flag itself
# This is the fixed scenario that used to return nothing

$ source <(./testprog completion bash)
# child2 has DisableFlagParsing == false
$ ./testprog child2 -[tab][tab]
# There are no flag name completion because Cobra does not know any

The same test can be run for zsh and fish, which both use a the Go completions (so a different code path than bash).

[umarcor]

Ref and possible conflict: #841.

[marckhouzam]

After thinking about it further, especially in the context of persistent flags, I took a more flexible and less intrusive approach.

If DisableFlagParsing==true, Cobra will still attempt to do the flag name completion, but will also call ValidArgsFunction. This allows Cobra to complete the persistent flags itself, and then request further flag name completions from ValidArgsFunction.

[marckhouzam]

Let me give a little more context for this fix.

Helm supports plugins. Each plugin is registered as a Cobra command. However, plugin sub-commands and plugin flags are not known to Helm/Cobra; they are handled directly by the plugin when it is executed. Therefore each plugin command has DisableFlagParsing=true.

Cobra not knowing about plugin flags implies it cannot perform flag-name completion. We must instead request the completion from the plugin itself and the best way to do that is through ValidArgsFunction which in turn would call the plugin.

This fix simply teaches Cobra to call ValidArgsFunction for flag-name completion when DisableFlagParsing=true.

A really cool scenario is then possible. For example, Helm has a plugin written using Cobra (helm-2to3). ValidArgsFunction could then call 2to3 __complete <args> to get all the plugin completions since Cobra (of the plugin) handles all that automatically. The missing part right now is flag-name completion triggering ValidArgsFunction

[marckhouzam]

Oh and since there are whispers about a new release of Cobra, maybe this fix can sneak in before?

[marckhouzam]

Please see description of the PR for a test program and testing steps.

[github-actions]

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

[marckhouzam]

Waiting for review.

[marckhouzam]

The diff looks much bigger than it really is because of indentation changes. If we ignore the indentation changes there are less than 20 lines of changes (if we ignore the test).

[jpmcb]

This is definitely something we want.

The missing part right now is flag-name completion triggering ValidArgsFunction

Can you elaborate on this @marckhouzam? Not sure I fully understand whats missing from this if we add in the calling of ValidArgsFunction for flags that have DisableFlagParsing true?

[marckhouzam]

This is definitely something we want.

The missing part right now is flag-name completion triggering ValidArgsFunction

Can you elaborate on this @marckhouzam? Not sure I fully understand whats missing from this if we add in the calling of ValidArgsFunction for flags that have DisableFlagParsing true?

Sorry @jpmcb, my comment was confusing. I meant to say that the missing part in Cobra right now (before this PR) is the calling of ValidArgsFunction for flags that have DisableFlagParsing true. It is fixed by this PR 😄.

So the PR is complete and ready.

[jpmcb]

This is looking good! Let's get it in!

[marckhouzam]

This is looking good! Let's get it in!

Did you mean to merge this @jpmcb or are you waiting for something from my side?

[jpmcb]

My bad, should have merged this earlier - lost some context on this but looks like it's good to go in