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

DisableFlagParsing must trigger custom completion for flag names #1161

Merged
merged 1 commit into from Nov 1, 2021

Conversation

marckhouzam
Copy link
Collaborator

@marckhouzam marckhouzam commented Jul 12, 2020

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
Copy link
Contributor

umarcor commented Jul 12, 2020

Ref and possible conflict: #841.

@marckhouzam
Copy link
Collaborator Author

marckhouzam commented Jul 13, 2020

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 marckhouzam changed the title DisableFlagParsing must disable flag completion DisableFlagParsing must trigger custom completion for flag names Jul 13, 2020
@marckhouzam
Copy link
Collaborator Author

marckhouzam commented Aug 15, 2020

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
Copy link
Collaborator Author

marckhouzam commented Aug 15, 2020

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

@marckhouzam
Copy link
Collaborator Author

marckhouzam commented Oct 9, 2020

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

@jharshman jharshman added this to the 2.0.0 milestone Oct 14, 2020
@github-actions
Copy link

github-actions bot commented Dec 14, 2020

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

@marckhouzam
Copy link
Collaborator Author

marckhouzam commented Dec 14, 2020

Waiting for review.

@jharshman jharshman modified the milestones: 2.0.0, Next Dec 20, 2020
@marckhouzam marckhouzam force-pushed the fix/compDisableFlags branch 2 times, most recently from 0043ab7 to cb96e4a Compare Feb 15, 2021
@marckhouzam marckhouzam force-pushed the fix/compDisableFlags branch 2 times, most recently from ca82f3b to 1341d6f Compare May 13, 2021
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.

This commit still tries to do flag name completion, but when
DisableFlagParsing==true, it continues on so that ValidArgsFunction will
be called; this allows the program to handle completion for its own
flags when DisableFlagParsing==true.

Signed-off-by: Marc Khouzam <marc.khouzam@montreal.ca>
@marckhouzam
Copy link
Collaborator Author

marckhouzam commented Jul 16, 2021

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
Copy link
Collaborator

jpmcb commented Aug 4, 2021

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?

@jpmcb jpmcb self-assigned this Aug 4, 2021
@marckhouzam
Copy link
Collaborator Author

marckhouzam commented Aug 4, 2021

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
jpmcb approved these changes Sep 10, 2021
Copy link
Collaborator

@jpmcb jpmcb left a comment

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

@marckhouzam
Copy link
Collaborator Author

marckhouzam commented Sep 16, 2021

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 jpmcb merged commit d2c0cb3 into spf13:master Nov 1, 2021
7 checks passed
@jpmcb
Copy link
Collaborator

jpmcb commented Nov 1, 2021

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

@marckhouzam marckhouzam deleted the fix/compDisableFlags branch Nov 1, 2021
scothis added a commit to scothis/tanzu-framework that referenced this issue Nov 29, 2021
This version includes spf13/cobra#1161 which
fixes an issue where completion options are not offered for plugin
flags.

Signed-off-by: Scott Andrews <andrewssc@vmware.com>
scothis added a commit to scothis/tanzu-framework that referenced this issue Dec 14, 2021
This version includes spf13/cobra#1161 which
fixes an issue where completion options are not offered for plugin
flags.

Signed-off-by: Scott Andrews <andrewssc@vmware.com>
scothis added a commit to scothis/tanzu-framework that referenced this issue Jan 11, 2022
This version includes spf13/cobra#1161 which
fixes an issue where completion options are not offered for plugin
flags.

Signed-off-by: Scott Andrews <andrewssc@vmware.com>
scothis added a commit to scothis/tanzu-framework that referenced this issue Jan 12, 2022
This version includes spf13/cobra#1161 which
fixes an issue where completion options are not offered for plugin
flags.

Signed-off-by: Scott Andrews <andrewssc@vmware.com>
vuil pushed a commit to vmware-tanzu/tanzu-framework that referenced this issue Jan 19, 2022
This version includes spf13/cobra#1161 which
fixes an issue where completion options are not offered for plugin
flags.

Signed-off-by: Scott Andrews <andrewssc@vmware.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants