Make command suggestion messages configurable

[zanvd]

This implements the proposal #1394.

[CLAassistant]

All committers have signed the CLA.

[ccoVeille]

Don't you need a something to check the parent has a suggestion func ?

Suggested change

	// SuggestFunc returns either the function set by SetSuggestFunc for this command
	// or a parent, or it returns a function with default suggestion behavior.
	func (c *Command) SuggestFunc() func(string) string {
	if c.suggestFunc != nil && !c.DisableSuggestions {
	return c.suggestFunc
	}
	if c.HasParent() {
	return c.Parent().SuggestFunc()
	}
	return c.findSuggestions
	}
	func (c *Command) HasSuggestFunc() bool {
	return c.suggestFunc != nil && !c.DisableSuggestions
	}
	// SuggestFunc returns either the function set by SetSuggestFunc for this command
	// or a parent, or it returns a function with default suggestion behavior.
	func (c *Command) SuggestFunc() func(string) string {
	if c.HasSuggestFunc() {
	return c.suggestFunc
	}
	if c.HasParent() && c.Parent().HasSuggestFunc() {
	return c.Parent().SuggestFunc()
	}
	return c.findSuggestions
	}

[zanvd]

This would be problematic from 2nd level onward (e.g. parent doesn't have it, but grandparent does).

Current logic would recursively check the whole parent tree and use the default (findSuggestions) if none is set.

[ccoVeille]

But, then

Won't this cause a problem?

https://github.com/spf13/cobra/blob/main/command.go#L752-L767

if you have a parent but no suggestFunc you will use the findSuggestion of the parent.
It's what you add in your code.

But then the suggestion will be computed only against the args supported by the parent, not by the one in the Command

[zanvd]

True, it would take the root's default suggestion function, which would fail to properly suggest a nested subcommand.

I can imagine someone wanting to bubble up to the root, though, and use a single custom suggestion function for the entire tree. So stopping early would be too limiting.

Will have to think about the best way to detect if it bubbled all the way to the root and use sucommand's parent (if it exists) when calling findSuggestions.

[zanvd]

Got the logic down, but it didn't have the desired effect. Checked how subcommands are detected and apparently you can't have a type on a lower level (e.g. grandchild).

• Find: after finding the last command detected in the args, it calls legacyArgs, where it checks for typos for root command, only, and returns an unknown command error with suggestions.
• Traverse: simply returns the last found command and doesn't check for possible typos (doesn't call legacyArgs).

Aside from Traverse not checking for possible typos (?), I'd say it makes sense, since an argument could very well be a valid one and not a typo.

Edit:
Will rework the SuggestFunc to work in this context.

There is OnlyValidArgs, though.

[ccoVeille]

It doesn't look fun, have fun !

[zanvd]

Reworked with the use of a helper function which gets called recursively until it finds the first custom suggestion function. If none is found, it's going to default to the parent's default.

I've also reworked the tests so they better test and represent the logic + added additional ones for the OnlyValidArgs path, which test the different levels of nesting.

[ccoVeille]

Using this

Suggested change

	func (c *Command) SuggestFunc() func(string) string {
	if c.suggestFunc != nil && !c.DisableSuggestions {
	return c.suggestFunc
	}
	if c.HasParent() {
	return c.Parent().SuggestFunc()
	}
	return c.findSuggestions
	}
	func (c *Command) SuggestFunc(arg string) string {
	if c.suggestFunc != nil && !c.DisableSuggestions {
	return c.suggestFunc(arg)
	}
	if c.HasParent() {
	return c.Parent().SuggestFunc(arg)
	}
	return c.findSuggestions(arg)
	}

Would avoid the strange way the function is called

-                                return fmt.Errorf("invalid argument %q for %q%s", v, cmd.CommandPath(), cmd.SuggestFunc()(args[0]))
+                                return fmt.Errorf("invalid argument %q for %q%s", v, cmd.CommandPath(), cmd.SuggestFunc(args[0]))

[zanvd]

Went with the return of a function because I wanted to mimic UsageFunc and HelpFunc logic as much as possible.

Pushed the simplified version.