From 10cf7be9972ee5b5994cf760ecba642309ac8685 Mon Sep 17 00:00:00 2001 From: Marc Khouzam Date: Mon, 24 Oct 2022 11:11:57 -0400 Subject: [PATCH] Check for group presence after full initialization (#1839) Fixes #1831 By moving the check for help group existence to "ExecuteC()" we no longer need groups to be added before AddCommand() is called. This provides more flexibility to developers and works better with the use of "init()" for command creation. Signed-off-by: Marc Khouzam --- command.go | 21 ++++++++++--- command_test.go | 78 +++++++++++++++++++++++++++++++++++++++++++++++++ user_guide.md | 9 +++--- 3 files changed, 100 insertions(+), 8 deletions(-) diff --git a/command.go b/command.go index 9d5e9cf..6ff47dd 100644 --- a/command.go +++ b/command.go @@ -998,6 +998,10 @@ func (c *Command) ExecuteC() (cmd *Command, err error) { // initialize completion at the last point to allow for user overriding c.InitDefaultCompletionCmd() + // Now that all commands have been created, let's make sure all groups + // are properly created also + c.checkCommandGroups() + args := c.args // Workaround FAIL with "go test -v" or "cobra.test -test.v", see #155 @@ -1092,6 +1096,19 @@ func (c *Command) ValidateRequiredFlags() error { return nil } +// checkCommandGroups checks if a command has been added to a group that does not exists. +// If so, we panic because it indicates a coding error that should be corrected. +func (c *Command) checkCommandGroups() { + for _, sub := range c.commands { + // if Group is not defined let the developer know right away + if sub.GroupID != "" && !c.ContainsGroup(sub.GroupID) { + panic(fmt.Sprintf("group id '%s' is not defined for subcommand '%s'", sub.GroupID, sub.CommandPath())) + } + + sub.checkCommandGroups() + } +} + // InitDefaultHelpFlag adds default help flag to c. // It is called automatically by executing the c or by calling help and usage. // If c already has help flag, it will do nothing. @@ -1218,10 +1235,6 @@ func (c *Command) AddCommand(cmds ...*Command) { panic("Command can't be a child of itself") } cmds[i].parent = c - // if Group is not defined let the developer know right away - if x.GroupID != "" && !c.ContainsGroup(x.GroupID) { - panic(fmt.Sprintf("Group id '%s' is not defined for subcommand '%s'", x.GroupID, cmds[i].CommandPath())) - } // update max lengths usageLen := len(x.Use) if usageLen > c.commandsMaxUseLen { diff --git a/command_test.go b/command_test.go index 0adec93..c023bd6 100644 --- a/command_test.go +++ b/command_test.go @@ -1862,6 +1862,84 @@ func TestAddGroup(t *testing.T) { checkStringContains(t, output, "\nTest group\n cmd") } +func TestWrongGroupFirstLevel(t *testing.T) { + var rootCmd = &Command{Use: "root", Short: "test", Run: emptyRun} + + rootCmd.AddGroup(&Group{ID: "group", Title: "Test group"}) + // Use the wrong group ID + rootCmd.AddCommand(&Command{Use: "cmd", GroupID: "wrong", Run: emptyRun}) + + defer func() { + if recover() == nil { + t.Errorf("The code should have panicked due to a missing group") + } + }() + _, err := executeCommand(rootCmd, "--help") + if err != nil { + t.Errorf("Unexpected error: %v", err) + } +} + +func TestWrongGroupNestedLevel(t *testing.T) { + var rootCmd = &Command{Use: "root", Short: "test", Run: emptyRun} + var childCmd = &Command{Use: "child", Run: emptyRun} + rootCmd.AddCommand(childCmd) + + childCmd.AddGroup(&Group{ID: "group", Title: "Test group"}) + // Use the wrong group ID + childCmd.AddCommand(&Command{Use: "cmd", GroupID: "wrong", Run: emptyRun}) + + defer func() { + if recover() == nil { + t.Errorf("The code should have panicked due to a missing group") + } + }() + _, err := executeCommand(rootCmd, "child", "--help") + if err != nil { + t.Errorf("Unexpected error: %v", err) + } +} + +func TestWrongGroupForHelp(t *testing.T) { + var rootCmd = &Command{Use: "root", Short: "test", Run: emptyRun} + var childCmd = &Command{Use: "child", Run: emptyRun} + rootCmd.AddCommand(childCmd) + + rootCmd.AddGroup(&Group{ID: "group", Title: "Test group"}) + // Use the wrong group ID + rootCmd.SetHelpCommandGroupID("wrong") + + defer func() { + if recover() == nil { + t.Errorf("The code should have panicked due to a missing group") + } + }() + _, err := executeCommand(rootCmd, "--help") + if err != nil { + t.Errorf("Unexpected error: %v", err) + } +} + +func TestWrongGroupForCompletion(t *testing.T) { + var rootCmd = &Command{Use: "root", Short: "test", Run: emptyRun} + var childCmd = &Command{Use: "child", Run: emptyRun} + rootCmd.AddCommand(childCmd) + + rootCmd.AddGroup(&Group{ID: "group", Title: "Test group"}) + // Use the wrong group ID + rootCmd.SetCompletionCommandGroupID("wrong") + + defer func() { + if recover() == nil { + t.Errorf("The code should have panicked due to a missing group") + } + }() + _, err := executeCommand(rootCmd, "--help") + if err != nil { + t.Errorf("Unexpected error: %v", err) + } +} + func TestSetOutput(t *testing.T) { c := &Command{} c.SetOutput(nil) diff --git a/user_guide.md b/user_guide.md index 6a686e3..fee9848 100644 --- a/user_guide.md +++ b/user_guide.md @@ -492,10 +492,11 @@ around it. In fact, you can provide your own if you want. ### Grouping commands in help -Cobra supports grouping of available commands. Groups must be explicitly defined by `AddGroup` and set by -the `GroupId` element of a subcommand. The groups will appear in the same order as they are defined. -If you use the generated `help` or `completion` commands, you can set the group ids by `SetHelpCommandGroupId` -and `SetCompletionCommandGroupId`, respectively. +Cobra supports grouping of available commands in the help output. To group commands, each group must be explicitly +defined using `AddGroup()` on the parent command. Then a subcommand can be added to a group using the `GroupID` element +of that subcommand. The groups will appear in the help output in the same order as they are defined using different +calls to `AddGroup()`. If you use the generated `help` or `completion` commands, you can set their group ids using +`SetHelpCommandGroupId()` and `SetCompletionCommandGroupId()` on the root command, respectively. ### Defining your own help