Check for group presence after full initialization (#1839) (#1841)

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 <marc.khouzam@gmail.com>

Signed-off-by: Marc Khouzam <marc.khouzam@gmail.com>
Co-authored-by: Marc Khouzam <marc.khouzam@gmail.com>

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 {

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)

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
+Cobra supports grouping of available commands in the help output.  To group commands, each group must be explicitly
-the `GroupId` element of a subcommand. The groups will appear in the same order as they are defined.
+defined using `AddGroup()` on the parent command.  Then a subcommand can be added to a group using the `GroupID` element
-If you use the generated `help` or `completion` commands, you can set the group ids by `SetHelpCommandGroupId`
+of that subcommand. The groups will appear in the help output in the same order as they are defined using different
-and `SetCompletionCommandGroupId`, respectively.
+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