Would like to be able to have ValidArgsFunction called when completing "-" instead of defaulting flag completion getCompletions()

[fnickels]

For some use cases of command line completion it would be helpful if there was a means to override the default behaviour of generating the completion list for flags.

# when hitting tab to generate command line completion ValidArgsFunction is called if present 
appcli command arg1 

# when hitting tab to generate command line completion getCompletions() handles the return values and ValidArgsFunction is not called
appcli command arg1 -

Most use cases of modifying the return list can be handled by leveraging MarkFlagsMutuallyExclusive(), but this only works for defined flags.

For context aware situations where the presence of a specific argument in the command line would modify which flags should be available it would be nice if getCompletions() called the command's ValidArgsFunction , which would allow the developer to customize the return list.

I can conceive of at least two implementations to add this functionality/feature to Cobra:

1. Change the default behaviour in getCompletions() such that whenever ValidArgsFunction is present it is always called. Likely not a good choice as it would break backward compatibility for the current implementation.

2. Introduce a new feature flag for commands to cause getCompletions() to call defer processing of a flag '-' to the command's ValidArgsFunction, if present. If ValidArgsFunction is not present, one of two behaviours could be entertained, either:

• return an error indicating the feature was enabled, but the function was not defined, or
• process the completion as it currently does.

The value of adding this capability is it provides the developer greater control of the command line completion results when the default behaviour does not satisfy the use case. The code written in ValidArgsFunction does not need any additional support to take advantage of this.

Example:

For the 'command' with three flags defined --flag1, --flag2, & --flag3 and the following:

		ValidArgsFunction: func(cmd *cobra.Command, args []string, toComplete string) ([]string, cobra.ShellCompDirective) {

			list := []string{}

			if len(args) == 0 {
				// add valid flags and presets
				list = append(list, "--flag1")
				list = append(list, "--flag2")
				list = append(list, "--flag3")
				list = append(list, "arg1")
				list = append(list, "arg2")
				list = append(list, "arg3")
			} else {
				list = append(list, "--flag1")
				list = append(list, "--flag2")
			}

			return list, cobra.ShellCompDirectiveNoFileComp
		},

# 'tab' returns  `--flag1`, `--flag2`
appcli command arg1 

# 'tab' returns  `--flag1`, `--flag2`, `--flag3`
appcli command arg1 -

A more sophisticated implementation might look like:

		ValidArgsFunction: func(cmd *cobra.Command, args []string, toComplete string) ([]string, cobra.ShellCompDirective) {

			list := []string{}

			if len(args) == 0 {
				// add valid flags and presets
				list = []string{ "arg1", "arg2", "arg3"} {
                                list = append( list, flagHintsRevised( cmd, []string{})...)
			} else {
				list = flagHintsRevised( cmd, "--flag3")
			}

			return list, cobra.ShellCompDirectiveNoFileComp
		},

using the support functions:

func flagHintsRevised(cmd *cobra.Command, forceRemovalList []string) []string {

	list := []string{}

	flagset := cmd.Flags()

	// has at least 1 non-hidden flag
	if !flagset.HasAvailableFlags() {
		return list
	}

	flagset.VisitAll(func(
		flag *pflag.Flag,
	) {
		if !flag.Hidden && !flag.Changed && !isFlagInList(flag, forceRemovalList) {
			if flag.Name != "" {
				list = append(list, "--" + flag.Name)
			}
			if flag.Shorthand != "" {
				list = append( "-" + flag.Shorthand)
			}
		}
	})

	return list
}

func isFlagInList(flag *pflag.Flag, list []string) bool {
	for _, v := range list {
		if v == flag.Name || v == flag.Shorthand {
			return true
		}
	}

	return false
}

[marckhouzam]

This makes sense to me @fnickels

Thinking quickly, another solution could be that instead of reusing ValidArgsFunction, we could add a new ValidFlagFunction. This approach may make it easier to reason about the list of completions to return since it would not require to pay attention if the first character is a - in ValidArgsFunction.

This approach would also preserve backwards compatibility.

One risk in not completing flags automatically though is that if a flag is added later on, the developer may forget to add it to its own custom completion function.

Thoughts?

[fnickels]

Interesting idea, I had not considered that.

In the various use cases I have used ValidArgsFunction in the past I have folded in (comingeled) both arguments and flags in the response to the function. For me this allows the end user of my CLIs to see all of the options, and when there are only flags left to add, the command line automatically advances to "-" on a tab. (The annoyance is the end user has to delete the exta "-" if they decide they are done with completions.)

Additionally, it is important to present required flags from this function so that and end user becomes aware of the required input when hitting tab.

Yet, this is my preference. I can foresee a different developer not wanting flags to be suggested with arguments, and hence your suggestion would be a useful way of segregation the logic for args and flags. I can also see cases where only required flags are shown, or a limited number of flags.

For my preference in command completion I would need to fold the logic for ValidFlagFunction into my ValidArgsFunction, when wanting to have the behavior show both valid args and flags. Which is not hard to do.

I also think your suggestion is cleaner implementation as a field to toggle the behavior is not required, and the call to this function would be more explicate in getCompletions(). It provides enough flexibility for a developer to choose how and when command line suggestions are intermixed while not complicating the implementation.

I can also see features added to decide if cobra comingles args and flags in the same command line completion response. That would cause developers such as me who have comingled outside of cobra to refactoring their code. I don't think that would be truly breaking backward compatability, as the new features only break things when added to a legacy app and the developer has the opportunity to refactoring when adding them.

I will try to find some time this week to post an alternate PR with your suggested solution. I like it better than what I originally came up with.