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

Make command suggestion messages configurable #2218

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions args.go
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ func legacyArgs(cmd *Command, args []string) error {

// root command with subcommands, do subcommand checking.
if !cmd.HasParent() && len(args) > 0 {
return fmt.Errorf("unknown command %q for %q%s", args[0], cmd.CommandPath(), cmd.findSuggestions(args[0]))
return fmt.Errorf("unknown command %q for %q%s", args[0], cmd.CommandPath(), cmd.SuggestFunc(args[0]))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does it have to be exported?

}
return nil
}
Expand All @@ -58,7 +58,7 @@ func OnlyValidArgs(cmd *Command, args []string) error {
}
for _, v := range args {
if !stringInSlice(v, validArgs) {
return fmt.Errorf("invalid argument %q for %q%s", v, cmd.CommandPath(), cmd.findSuggestions(args[0]))
return fmt.Errorf("invalid argument %q for %q%s", v, cmd.CommandPath(), cmd.SuggestFunc(args[0]))
}
}
}
Expand Down
37 changes: 37 additions & 0 deletions command.go
Original file line number Diff line number Diff line change
Expand Up @@ -180,6 +180,8 @@ type Command struct {
helpCommand *Command
// helpCommandGroupID is the group id for the helpCommand
helpCommandGroupID string
// suggestFunc is suggest func defined by the user.
suggestFunc func(string) string

// completionCommandGroupID is the group id for the completion command
completionCommandGroupID string
Expand Down Expand Up @@ -340,6 +342,10 @@ func (c *Command) SetHelpCommandGroupID(groupID string) {
c.helpCommandGroupID = groupID
}

func (c *Command) SetSuggestFunc(f func(string) string) {
c.suggestFunc = f
}

// SetCompletionCommandGroupID sets the group id of the completion command.
func (c *Command) SetCompletionCommandGroupID(groupID string) {
// completionCommandGroupID is used if no completion command is defined by the user
Expand Down Expand Up @@ -477,6 +483,37 @@ func (c *Command) Help() error {
return nil
}

// SuggestFunc returns suggestions for the provided typedName using either
// the function set by SetSuggestFunc for this command, parent's or a default one.
// When searching for a parent's function, it recursively checks towards the root
// and returns the first one found. If none found, uses direct parent's default.
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 {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 {

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

@ccoVeille ccoVeille Jan 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@ccoVeille ccoVeille Jan 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ?

Copy link
Contributor

@ccoVeille ccoVeille Jan 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

@zanvd zanvd Jan 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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. 🙂

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


// UsageString returns usage string.
func (c *Command) UsageString() string {
// Storing normal writers
Expand Down
120 changes: 120 additions & 0 deletions command_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -1393,6 +1393,126 @@ func TestSuggestions(t *testing.T) {
}
}

func TestCustomSuggestions(t *testing.T) {
rootCmd := &Command{Use: "root", Run: emptyRun}
timesCmd := &Command{Use: "times", Run: emptyRun}
rootCmd.AddCommand(timesCmd)

var expected, output string

expected = ""
output, _ = executeCommand(rootCmd, "times")
if output != expected {
t.Errorf("\nExpected:\n %q\nGot:\n %q", expected, output)
}

expected = fmt.Sprintf("Error: unknown command \"%s\" for \"root\"\n\nDid you mean this?\n\t%s\n\nRun 'root --help' for usage.\n", "time", "times")
output, _ = executeCommand(rootCmd, "time")
if output != expected {
t.Errorf("\nExpected:\n %q\nGot:\n %q", expected, output)
}

rootCmd.DisableSuggestions = true

expected = fmt.Sprintf("Error: unknown command \"%s\" for \"root\"\nRun 'root --help' for usage.\n", "time")
output, _ = executeCommand(rootCmd, "time")
if output != expected {
t.Errorf("\nExpected:\n %q\nGot:\n %q", expected, output)
}

rootCmd.DisableSuggestions = false
rootCmd.SetSuggestFunc(func(typedName string) string {
return "\nSome custom suggestion.\n"
})

expected = fmt.Sprintf("Error: unknown command \"%s\" for \"root\"\nSome custom suggestion.\n\nRun 'root --help' for usage.\n", "time")
output, _ = executeCommand(rootCmd, "time")
if output != expected {
t.Errorf("\nExpected:\n %q\nGot:\n %q", expected, output)
}
}

func TestCustomSuggestions_OnlyValidArgs(t *testing.T) {
validArgs := []string{"a"}
rootCmd := &Command{Use: "root", Args: OnlyValidArgs, Run: emptyRun, ValidArgs: validArgs}
grandparentCmd := &Command{Use: "grandparent", Args: OnlyValidArgs, Run: emptyRun, ValidArgs: validArgs}
parentCmd := &Command{Use: "parent", Args: OnlyValidArgs, Run: emptyRun, ValidArgs: validArgs}
timesCmd := &Command{Use: "times", Run: emptyRun}
parentCmd.AddCommand(timesCmd)
grandparentCmd.AddCommand(parentCmd)
rootCmd.AddCommand(grandparentCmd)

var expected, output string

// No typos.
expected = ""
output, _ = executeCommand(rootCmd, "grandparent")
if output != expected {
t.Errorf("\nExpected:\n %q\nGot:\n %q", expected, output)
}

expected = ""
output, _ = executeCommand(rootCmd, "grandparent", "parent")
if output != expected {
t.Errorf("\nExpected:\n %q\nGot:\n %q", expected, output)
}

expected = ""
output, _ = executeCommand(rootCmd, "grandparent", "parent", "times")
if output != expected {
t.Errorf("\nExpected:\n %q\nGot:\n %q", expected, output)
}

// 1st level typo.
expected = "Error: invalid argument \"grandparen\" for \"root\"\n\nDid you mean this?\n\tgrandparent\n\nUsage:\n root [flags]\n root [command]\n\nAvailable Commands:\n completion Generate the autocompletion script for the specified shell\n grandparent \n help Help about any command\n\nFlags:\n -h, --help help for root\n\nUse \"root [command] --help\" for more information about a command.\n\n"
output, _ = executeCommand(rootCmd, "grandparen")
if output != expected {
t.Errorf("\nExpected:\n %q\nGot:\n %q", expected, output)
}

// 2nd level typo.
expected = "Error: invalid argument \"paren\" for \"root grandparent\"\nUsage:\n root grandparent [flags]\n root grandparent [command]\n\nAvailable Commands:\n parent \n\nFlags:\n -h, --help help for grandparent\n\nUse \"root grandparent [command] --help\" for more information about a command.\n\n"
output, _ = executeCommand(rootCmd, "grandparent", "paren")
if output != expected {
t.Errorf("\nExpected:\n %q\nGot:\n %q", expected, output)
}

// 3rd level typo.
expected = "Error: invalid argument \"time\" for \"root grandparent parent\"\nUsage:\n root grandparent parent [flags]\n root grandparent parent [command]\n\nAvailable Commands:\n times \n\nFlags:\n -h, --help help for parent\n\nUse \"root grandparent parent [command] --help\" for more information about a command.\n\n"
output, _ = executeCommand(rootCmd, "grandparent", "parent", "time")
if output != expected {
t.Errorf("\nExpected:\n %q\nGot:\n %q", expected, output)
}

// Custom suggestion on root function.
rootCmd.SetSuggestFunc(func(typedName string) string {
return "\nRoot custom suggestion.\n"
})

expected = "Error: invalid argument \"grandparen\" for \"root\"\nRoot custom suggestion.\n\nUsage:\n root [flags]\n root [command]\n\nAvailable Commands:\n completion Generate the autocompletion script for the specified shell\n grandparent \n help Help about any command\n\nFlags:\n -h, --help help for root\n\nUse \"root [command] --help\" for more information about a command.\n\n"
output, _ = executeCommand(rootCmd, "grandparen")
if output != expected {
t.Errorf("\nExpected:\n %q\nGot:\n %q", expected, output)
}

expected = "Error: invalid argument \"time\" for \"root grandparent parent\"\nRoot custom suggestion.\n\nUsage:\n root grandparent parent [flags]\n root grandparent parent [command]\n\nAvailable Commands:\n times \n\nFlags:\n -h, --help help for parent\n\nUse \"root grandparent parent [command] --help\" for more information about a command.\n\n"
output, _ = executeCommand(rootCmd, "grandparent", "parent", "time")
if output != expected {
t.Errorf("\nExpected:\n %q\nGot:\n %q", expected, output)
}

// Custom suggestion on parent function (kept root's to make sure this one is prioritised).
parentCmd.SetSuggestFunc(func(typedName string) string {
return "\nParent custom suggestion.\n"
})

expected = "Error: invalid argument \"time\" for \"root grandparent parent\"\nParent custom suggestion.\n\nUsage:\n root grandparent parent [flags]\n root grandparent parent [command]\n\nAvailable Commands:\n times \n\nFlags:\n -h, --help help for parent\n\nUse \"root grandparent parent [command] --help\" for more information about a command.\n\n"
output, _ = executeCommand(rootCmd, "grandparent", "parent", "time")
if output != expected {
t.Errorf("\nExpected:\n %q\nGot:\n %q", expected, output)
}
}

func TestCaseInsensitive(t *testing.T) {
rootCmd := &Command{Use: "root", Run: emptyRun}
childCmd := &Command{Use: "child", Run: emptyRun, Aliases: []string{"alternative"}}
Expand Down
Loading