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.

Sign up for GitHub

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

Jump to bottom

closes #2130 new command flag ErrorOnUnknownSubcommand #2167

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

closes #2130 new command flag ErrorOnUnknownSubcommand #2167

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
9892605
closes #2130 new command flag ErrorOnUnknownSubcommand
andremueller Jul 6, 2024
3974349
fix linting issue
andremueller Jul 8, 2024
133c649
moved to global EnableErrorOnUnknownSubcommand option
andremueller Jul 9, 2024
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
22 changes: 20 additions & 2 deletions command.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,10 @@ type Group struct {
Title string
}

// ErrUnknownSubcommand is returned by Command.Execute() when the subcommand was not found.
// Hereto, the ErrorOnUnknownSubcommand flag must be set true.
var ErrUnknownSubcommand = errors.New("subcommand is unknown")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's format this command message like the one in legacyArgs() to have a consistent behaviour.
fmt.Errorf("unknown command %q for %q%s", args[0], cmd.CommandPath(), cmd.findSuggestions(args[0]))


// Command is just that, a command for your application.
// E.g. 'go run ...' - 'run' is the command. Cobra requires
// you to define the usage and description as part of your command
Expand Down Expand Up @@ -254,6 +258,17 @@ type Command struct {
// SuggestionsMinimumDistance defines minimum levenshtein distance to display suggestions.
// Must be > 0.
SuggestionsMinimumDistance int

// ErrorOnUnknownSubcommand controls the behavior of subcommand handling.
// When the flag is set true the behavior of Command.Execute() will change:
// If a sub-subcommand is not found ErrUnknownSubcommand will be returned on calling
// Command.Exec() instead of the old behavior where a nil error was sent.
// If the flag is false (default) the old behavior is performed.
// For this behavior the child subcommand must be nil.
// Example: in root/service/run - there would be an existing subcommand `run`
// in root/service/unknown - there would be an unknown subcommand `unknown` therefore returning an error
// `service` must have a nil Command.Run() function for this.
ErrorOnUnknownSubcommand bool
}

// Context returns underlying command context. If command was executed
Expand Down Expand Up @@ -923,9 +938,12 @@ func (c *Command) execute(a []string) (err error) {
}

if !c.Runnable() {
return flag.ErrHelp
if c.ErrorOnUnknownSubcommand {
Copy link
Collaborator

Choose a reason for hiding this comment

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

c is the current command here. Therefore I would need to set this new flag for every command throughout my program to get the new behaviour globally.

Do you feel this feature should be enabled per command or globally?

If globally, then you can add a global variable in cobra.go instead of in the command structure. Then the program can enable this feature in a single location.

Copy link
Author

Choose a reason for hiding this comment

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

Hey there! You are right. I think the global behavior is more what I (and probably others) want! I will change that!

Copy link
Author

Choose a reason for hiding this comment

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

Hi @marckhouzam. Did you have a chance to review my changes? Would the behavior be so ok?

return ErrUnknownSubcommand
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's only return the error if there is at least one arg.
This will allow to get the usage printed without an error when running a valid command that doesn't have a Run. This is something users do a lot. For example running helm repo should print the usage without showing an error.

You should be able to move up the following lines to get the number of args:

argWoFlags := c.Flags().Args()
	if c.DisableFlagParsing {
		argWoFlags = a
	}

} else {
return flag.ErrHelp
}
}

c.preRun()

defer c.postRun()
Expand Down
89 changes: 89 additions & 0 deletions command_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
import (
"bytes"
"context"
"errors"
"fmt"
"io"
"os"
Expand Down Expand Up @@ -169,11 +170,99 @@
t.Errorf("Unexpected error: %v", err)
}

if c.Name() != "child" {

Check failure on line 173 in command_test.go

View workflow job for this annotation

GitHub Actions / golangci-lint 

string `child` has 5 occurrences, make it a constant (goconst)
t.Errorf(`invalid command returned from ExecuteC: expected "child"', got: %q`, c.Name())
}
}

func TestSubcommandExecuteMissingSubcommand(t *testing.T) {
rootCmd := &Command{Use: "root", Run: emptyRun}
childCmd := &Command{Use: "child", Run: nil, ErrorOnUnknownSubcommand: false}
child2Cmd := &Command{Use: "child2", Run: emptyRun}
rootCmd.AddCommand(childCmd)
childCmd.AddCommand(child2Cmd)

// test existing command
c, output, err := executeCommandC(rootCmd, "child")
if !strings.HasPrefix(output, "Usage:") {
t.Errorf("Unexpected output: %v", output)
}
if err != nil {
t.Errorf("Unexpected error: %v", err)
}
if c.Name() != "child" {
t.Errorf(`invalid command returned from ExecuteC: expected "child"', got: %q`, c.Name())
}

// test existing sub command
c, output, err = executeCommandC(rootCmd, "child", "child2")
if output != "" {
t.Errorf("Unexpected output: %v", output)
}
if err != nil {
t.Errorf("Unexpected error: %v", err)
}
if c.Name() != "child2" {
t.Errorf(`invalid command returned from ExecuteC: expected "child"', got: %q`, c.Name())
}

// now test a command which does not exist, we will get no error, just "Usage:" is printed
c, output, err = executeCommandC(rootCmd, "child", "unknownChild")
if !strings.HasPrefix(output, "Usage:") {
t.Errorf("Expected: 'Usage: ...'\nGot:\n %q\n", output)
}
if err != nil {
t.Errorf("Unexpected error: %v", err)
}
if c.Name() != "child" {
t.Errorf(`invalid command returned from ExecuteC: expected "child"', got: %q`, c.Name())
}
}

func TestSubcommandExecuteMissingSubcommandWithErrorOnUnknownSubcommand(t *testing.T) {
rootCmd := &Command{Use: "root", Run: emptyRun}
childCmd := &Command{Use: "child", Run: nil, ErrorOnUnknownSubcommand: true}
child2Cmd := &Command{Use: "child2", Run: emptyRun}
rootCmd.AddCommand(childCmd)
childCmd.AddCommand(child2Cmd)

// test existing command
c, output, err := executeCommandC(rootCmd, "child")
if !strings.HasPrefix(output, "Error:") {
t.Errorf("Unexpected output: %v", output)
}
if !errors.Is(err, ErrUnknownSubcommand) {
t.Errorf("Unexpected error: %v", err)
}
if c.Name() != "child" {
t.Errorf(`invalid command returned from ExecuteC: expected "child"', got: %q`, c.Name())
}

// test existing sub command
c, output, err = executeCommandC(rootCmd, "child", "child2")
if output != "" {
t.Errorf("Unexpected output: %v", output)
}
if err != nil {
t.Errorf("Unexpected error: %v", err)
}
if c.Name() != "child2" {
t.Errorf(`invalid command returned from ExecuteC: expected "child"', got: %q`, c.Name())
}

// now test a command which does not exist, we expect an error because of the ErrorOnUnknownSubcommand flag
c, output, err = executeCommandC(rootCmd, "child", "unknownChild")
if !strings.HasPrefix(output, "Error:") {
t.Errorf("Unexpected output: %v", output)
}
if !errors.Is(err, ErrUnknownSubcommand) {
t.Errorf("Unexpected error: %v", err)
}
if c.Name() != "child" {
t.Errorf(`invalid command returned from ExecuteC: expected "child"', got: %q`, c.Name())
}
}

func TestExecuteContext(t *testing.T) {
ctx := context.TODO()

Expand Down
Loading