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 5 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 1 commit
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]))
}
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
18 changes: 18 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,18 @@ func (c *Command) Help() error {
return nil
}

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

Choose a reason for hiding this comment

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

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
}

Copy link
Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@ccoVeille ccoVeille Jan 17, 2025

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

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.

Copy link
Author

@zanvd zanvd Jan 18, 2025

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

It doesn't look fun, have fun !

Copy link
Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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]))

Copy link
Author

Choose a reason for hiding this comment

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

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

Pushed the simplified version.


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

func TestCustomSuggestions(t *testing.T) {
templateWithCustomSuggestions := "Error: unknown command \"%s\" for \"root\"\nSome custom suggestion.\n\nRun 'root --help' for usage.\n"
templateWithDefaultSuggestions := "Error: unknown command \"%s\" for \"root\"\n\nDid you mean this?\n\t%s\n\nRun 'root --help' for usage.\n"
templateWithoutSuggestions := "Error: unknown command \"%s\" for \"root\"\nRun 'root --help' for usage.\n"

for typo, suggestion := range map[string]string{"time": "times", "timse": "times"} {
for _, suggestionsDisabled := range []bool{true, false} {
for _, setCustomSuggest := range []bool{true, false} {
rootCmd := &Command{Use: "root", Run: emptyRun}
timesCmd := &Command{
Use: "times",
Run: emptyRun,
}
rootCmd.AddCommand(timesCmd)

rootCmd.DisableSuggestions = suggestionsDisabled

if setCustomSuggest {
rootCmd.SetSuggestFunc(func(a string) string {
return "\nSome custom suggestion.\n"
})
}

var expected string
if suggestionsDisabled {
expected = fmt.Sprintf(templateWithoutSuggestions, typo)
} else if setCustomSuggest {
expected = fmt.Sprintf(templateWithCustomSuggestions, typo)
} else {
expected = fmt.Sprintf(templateWithDefaultSuggestions, typo, suggestion)
}

output, _ := executeCommand(rootCmd, typo)

if output != expected {
t.Errorf("Unexpected response.\nExpected:\n %q\nGot:\n %q\n", 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