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.

[ccoVeille]

You can inline it, no?

Suggested change

	var getParentFunc func(*Command) func(string) string
	getParentFunc = func(parent *Command) func(string) string {
	getParentFunc := func(parent *Command) func(string) string {

[zanvd]

No, have to define it separately because of the recursion.

[ccoVeille]

This might be more readable

Suggested change

	if parent.HasParent() {
	return getParentFunc(parent.Parent())
	}
	return nil
	if !parent.HasParent() {
	return nil
	}
	return getParentFunc(parent.Parent())

[zanvd]

Done.

[ccoVeille]

There are still something strange here to me

If you ask for a suggestion for a command defined on a parent.

Shouldn't we append them all in fact?

Please note I'm pretty sure I'm asking to resolve something that was broken before you started working on it.

So the whole code should be about calling every possible things in the stack above the command.

Suggested change

	func (c *Command) SuggestFunc(typedName string) string {
	if c.DisableSuggestions {
	return ""
	}
	if c.suggestFunc != nil {
	return c.suggestFunc(typedName)
	}
	if c.HasParent() {
	var getParentFunc func(*Command) func(string) string
	getParentFunc = func(parent *Command) func(string) string {
	if parent.suggestFunc != nil {
	return parent.suggestFunc
	}
	if !parent.HasParent() {
	return nil
	}
	return getParentFunc(parent.Parent())
	}
	parentFunc := getParentFunc(c.Parent())
	if parentFunc != nil {
	return parentFunc(typedName)
	}
	return c.Parent().findSuggestions(typedName)
	}
	return c.findSuggestions(typedName)
	}
	func (c *Command) SuggestFunc(typedName string) string {
	if c.DisableSuggestions {
	return ""
	}
	suggestFunc := c.findSuggestions
	if c.suggestFunc != nil {
	suggestFunc = c.suggestFunc
	}
	suggestions := suggestFunc(typedName)
	if c.HasParent() {
	suggestions += c.Parent().SuggestFunc(typedName)
	}
	return suggestions
	}

The only things that worry my now, is the fact the "did you mean ?" that findSuggestions might be repeated, but it could be moved here

Suggested change

	func (c *Command) SuggestFunc(typedName string) string {
	if c.DisableSuggestions {
	return ""
	}
	if c.suggestFunc != nil {
	return c.suggestFunc(typedName)
	}
	if c.HasParent() {
	var getParentFunc func(*Command) func(string) string
	getParentFunc = func(parent *Command) func(string) string {
	if parent.suggestFunc != nil {
	return parent.suggestFunc
	}
	if !parent.HasParent() {
	return nil
	}
	return getParentFunc(parent.Parent())
	}
	parentFunc := getParentFunc(c.Parent())
	if parentFunc != nil {
	return parentFunc(typedName)
	}
	return c.Parent().findSuggestions(typedName)
	}
	return c.findSuggestions(typedName)
	}
	func (c *Command) SuggestFunc(typedName string) string {
	if c.DisableSuggestions {
	return ""
	}
	suggestFunc := c.findSuggestions
	if c.suggestFunc != nil {
	suggestFunc = c.suggestFunc
	}
	suggestions := suggestFunc(typedName)
	if c.HasParent() {
	suggestions += c.Parent().SuggestFunc(typedName)
	}
	if suggestions == "" {
	return ""
	}
	return "Did you mean?\n"+suggestions
	}

This is pure pseudocode written on my phone, but I think you get the idea

[ccoVeille]

Another way to consider this is to keep calling findSuggestions in args.go and add the logic of SuggestFunc there with the overload of the parent

My point is the fact that the suggestion func you add could/should be there

https://github.com/spf13/cobra/blob/01ffff4eca5a08384ef2b85f39ec0dac192a5f7b/command.go#L760C2-L765C3

and provide []string of suggestions, so the logic of calling the parent could be added there to append elements to this array.

Said otherwise, what you did is OK, but I'm afraid the code that was present and you updated was wrong.

What do you think ?

[ccoVeille]

The more I read #1394

Hi, we have a project where clear definitions were made, what the output of the CLI has to look like. We appreciate the automatic suggestions if a command was misspelled. Sadly it seems like we cannot customize the suggestion message and the default message doesn't really fit with the rest of the CLI.

the more I'm wondering if the issue was not about having a method to replace this

		sb.WriteString("\n\nDid you mean this?\n")
		for _, s := range suggestions {
			_, _ = fmt.Fprintf(&sb, "\t%v\n", s)
		}

So having a method to do this only (plus a setter for suggestFunc

func (c *Command) findSuggestions(arg string) string {
	if c.DisableSuggestions {
		return ""
	}
	if c.SuggestionsMinimumDistance <= 0 {
		c.SuggestionsMinimumDistance = 2
	}
	suggestions := c.SuggestionsFor(arg)

	if  len(suggestions) == 0 {
		return ""
	}

	if c.suggestFunc != nil {
		return c.suggestFunc(suggestions)
	}

	var sb strings.Builder
	sb.WriteString("\n\nDid you mean this?\n")
	for _, s := range suggestions {
		_, _ = fmt.Fprintf(&sb, "\t%v\n", s)
	}
	return sb.String()
}

I don't think people want to implement the computing what should be suggested, or calling SuggestFor in their own implementation

[zanvd]

I've wanted to keep it as unopinionated as possible, in the same manner HelpFunc and UsageFunc are. This would cover more use cases and have a lower chance of extending the functionality further at a later time. Additionally, the SuggestionsFor method is already exported and there's little to no overhead for the user if he has to call it on his own.

I don't mind going with what you've suggested, but would prefer to let the user override the whole thing.

Edit: If it's fine with you, I'll wait until we set on the final version of requirements before answering your other comments as they may become obsolete. 🙂

[ccoVeille]

I don't mind going with what you've suggested, but would prefer to let the user override the whole thing.

Don't get me wrong. What you provided here is great. It's just that's not what was requested in the issue you mentioned.

My point is that these are two distinct feature and need.

So having a distinct setter SetSuggestFunc that does what you did, and another setter that will be about taking a []string of suggestions and format it the way the user want.

I don't think most people want to recode the levenstein computing. I mean right now, with the code in your PR would imply for them to call the SuggestedFor code.

Maybe I'm over thinking everything.

My feedbacks are only mine. Other people may think differently, and I'm fine with this.

[zanvd]

I'll simplify it and go with just the override of the output, as you've suggested.

[ccoVeille]

Does it have to be exported?

[ccoVeille]

The PR seems to go out of what is requested in the issue.

It might be OK, but I prefer to put a "requested changes" status for now

[zanvd]

@ccoVeille, I've pushed the rework and want to note a couple of things:

• Kept recursive search for the override on parents: wanted to allow for a single override for any number of subcommands on any level.
• Kept search for override within the current branch, only: branching out would prevent users to define custom suggestion output on a limited set of commands.

[zanvd]

Thank you for the review and suggestions @ccoVeille!