Address comments

Signed-off-by: Marc Khouzam <marc.khouzam@gmail.com>
spf13 · Jun 11, 2024 · 9fbba57 · 9fbba57
1 parent 8e51700
commit 9fbba57
Show file tree

Hide file tree

Showing 2 changed files with 104 additions and 99 deletions.
diff --git a/command.go b/command.go
@@ -469,10 +469,8 @@ func (c *Command) HelpFunc() func(*Command, []string) {
 	}
 }
 
-// Help puts out the help for the command.
+// Help invokes the HelpFunc without arguments.
 // Kept for backwards compatibility.
-// No longer used because it does not allow
-// to pass arguments to the help command.
 // Can be a simple way to trigger the help
 // if arguments are not needed.
 func (c *Command) Help() error {
@@ -1124,11 +1122,14 @@ func (c *Command) ExecuteC() (cmd *Command, err error) {
 		if errors.Is(err, flag.ErrHelp) {
 			// The call to execute() above has parsed the flags.
 			// We therefore only pass the remaining arguments to the help function.
-			argWoFlags := cmd.Flags().Args()
+			remainingArgs := cmd.Flags().Args()
 			if cmd.DisableFlagParsing {
-				argWoFlags = flags
+				// For commands that have DisableFlagParsing == true, the flag parsing
+				// was not done and cmd.Flags().Args() is not filled.  We therefore
+				// use the full set of arguments, which include flags.
+				remainingArgs = flags
 			}
-			cmd.HelpFunc()(cmd, argWoFlags)
+			cmd.HelpFunc()(cmd, remainingArgs)
 			return cmd, nil
 		}
 

diff --git a/command_test.go b/command_test.go
@@ -12,7 +12,6 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.
 
-//nolint:goconst
 package cobra
 
 import (
@@ -1080,189 +1079,194 @@ func TestHelpExecutedOnNonRunnableChild(t *testing.T) {
 	checkStringContains(t, output, childCmd.Long)
 }
 
-func TestHelpOverrideOnRoot(t *testing.T) {
-	var helpCalled bool
-	rootCmd := &Command{Use: "root"}
-	rootCmd.SetHelpFunc(func(c *Command, args []string) {
-		helpCalled = true
-		if c.Name() != "root" {
-			t.Errorf(`Expected command name: "root", got %q`, c.Name())
+type overridingHelp struct {
+	expectedCmdName string
+	expectedArgs    []string
+
+	helpCalled bool
+	err        error
+}
+
+func (o *overridingHelp) helpFunc() func(c *Command, args []string) {
+	return func(c *Command, args []string) {
+		o.helpCalled = true
+		if c.Name() != o.expectedCmdName {
+			o.err = fmt.Errorf("Expected command name: %q, got %q", o.expectedCmdName, c.Name())
+			return
+		}
+
+		if len(args) != len(o.expectedArgs) {
+			o.err = fmt.Errorf("Expected args %v, got %v", o.expectedArgs, args)
+			return
 		}
-		if len(args) != 2 || args[0] != "arg1" || args[1] != "arg2" {
-			t.Errorf("Expected args [args1 arg2], got %v", args)
+
+		for i, arg := range o.expectedArgs {
+			if args[i] != arg {
+				o.err = fmt.Errorf("Expected args %v, got %v", o.expectedArgs, args)
+				return
+			}
 		}
-	})
+	}
+}
+
+func (o *overridingHelp) checkError() error {
+	if o.err != nil {
+		return o.err
+	}
+	if !o.helpCalled {
+		return fmt.Errorf("Overridden help function not called")
+	}
+	return nil
+}
+
+func TestHelpOverrideOnRoot(t *testing.T) {
+	rootCmd := &Command{Use: "root"}
+
+	override := overridingHelp{
+		expectedCmdName: rootCmd.Name(),
+		expectedArgs:    []string{"arg1", "arg2"},
+	}
+	rootCmd.SetHelpFunc(override.helpFunc())
 
 	_, err := executeCommand(rootCmd, "arg1", "arg2", "--help")
 	if err != nil {
-		t.Errorf("Unexpected error: %v", err)
+		t.Errorf("Unexpected error executing command: %v", err)
 	}
 
-	if !helpCalled {
-		t.Error("Overridden help function not called")
+	if err = override.checkError(); err != nil {
+		t.Errorf("Unexpected error from help function: %v", err)
 	}
 }
 
 func TestHelpOverrideOnChild(t *testing.T) {
-	var helpCalled bool
 	rootCmd := &Command{Use: "root"}
 	subCmd := &Command{Use: "child"}
 	rootCmd.AddCommand(subCmd)
 
-	subCmd.SetHelpFunc(func(c *Command, args []string) {
-		helpCalled = true
-		if c.Name() != "child" {
-			t.Errorf(`Expected command name: "child", got %q`, c.Name())
-		}
-		if len(args) != 2 || args[0] != "arg1" || args[1] != "arg2" {
-			t.Errorf("Expected args [args1 arg2], got %v", args)
-		}
-	})
+	override := overridingHelp{
+		expectedCmdName: subCmd.Name(),
+		expectedArgs:    []string{"arg1", "arg2"},
+	}
+	subCmd.SetHelpFunc(override.helpFunc())
 
 	_, err := executeCommand(rootCmd, "child", "arg1", "arg2", "--help")
 	if err != nil {
-		t.Errorf("Unexpected error: %v", err)
+		t.Errorf("Unexpected error executing command: %v", err)
 	}
 
-	if !helpCalled {
-		t.Error("Overridden help function not called")
+	if err = override.checkError(); err != nil {
+		t.Errorf("Unexpected error from help function: %v", err)
 	}
 }
 
 func TestHelpOverrideOnRootWithChild(t *testing.T) {
-	var helpCalled bool
 	rootCmd := &Command{Use: "root"}
 	subCmd := &Command{Use: "child"}
 	rootCmd.AddCommand(subCmd)
 
-	rootCmd.SetHelpFunc(func(c *Command, args []string) {
-		helpCalled = true
-		if c.Name() != "child" {
-			t.Errorf(`Expected command name: "child", got %q`, c.Name())
-		}
-		if len(args) != 2 || args[0] != "arg1" || args[1] != "arg2" {
-			t.Errorf("Expected args [args1 arg2], got %v", args)
-		}
-	})
+	override := overridingHelp{
+		expectedCmdName: subCmd.Name(),
+		expectedArgs:    []string{"arg1", "arg2"},
+	}
+	rootCmd.SetHelpFunc(override.helpFunc())
 
 	_, err := executeCommand(rootCmd, "child", "arg1", "arg2", "--help")
 	if err != nil {
-		t.Errorf("Unexpected error: %v", err)
+		t.Errorf("Unexpected error executing command: %v", err)
 	}
 
-	if !helpCalled {
-		t.Error("Overridden help function not called")
+	if err = override.checkError(); err != nil {
+		t.Errorf("Unexpected error from help function: %v", err)
 	}
 }
 
 func TestHelpOverrideOnRootWithChildAndFlags(t *testing.T) {
-	var helpCalled bool
 	rootCmd := &Command{Use: "root"}
 	subCmd := &Command{Use: "child"}
 	rootCmd.AddCommand(subCmd)
 
 	var myFlag bool
 	subCmd.Flags().BoolVar(&myFlag, "myflag", false, "")
 
-	rootCmd.SetHelpFunc(func(c *Command, args []string) {
-		helpCalled = true
-		if c.Name() != "child" {
-			t.Errorf(`Expected command name: "child", got %q`, c.Name())
-		}
-		if len(args) != 2 || args[0] != "arg1" || args[1] != "arg2" {
-			t.Errorf("Expected args [args1 arg2], got %v", args)
-		}
-	})
+	override := overridingHelp{
+		expectedCmdName: subCmd.Name(),
+		expectedArgs:    []string{"arg1", "arg2"},
+	}
+	rootCmd.SetHelpFunc(override.helpFunc())
 
 	_, err := executeCommand(rootCmd, "child", "arg1", "--myflag", "arg2", "--help")
 	if err != nil {
-		t.Errorf("Unexpected error: %v", err)
+		t.Errorf("Unexpected error executing command: %v", err)
 	}
 
-	if !helpCalled {
-		t.Error("Overridden help function not called")
+	if err = override.checkError(); err != nil {
+		t.Errorf("Unexpected error from help function: %v", err)
 	}
 }
 
 func TestHelpOverrideOnRootWithChildAndFlagsButParsingDisabled(t *testing.T) {
-	var helpCalled bool
 	rootCmd := &Command{Use: "root"}
 	subCmd := &Command{Use: "child", DisableFlagParsing: true}
 	rootCmd.AddCommand(subCmd)
 
 	var myFlag bool
 	subCmd.Flags().BoolVar(&myFlag, "myflag", false, "")
 
-	rootCmd.SetHelpFunc(func(c *Command, args []string) {
-		helpCalled = true
-		if c.Name() != "child" {
-			t.Errorf(`Expected command name: "child", got %q`, c.Name())
-		}
-		if len(args) != 4 ||
-			args[0] != "arg1" || args[1] != "--myflag" || args[2] != "arg2" || args[3] != "--help" {
-			t.Errorf("Expected args [args1 --myflag arg2 --help], got %v", args)
-		}
-	})
+	override := overridingHelp{
+		expectedCmdName: subCmd.Name(),
+		expectedArgs:    []string{"arg1", "--myflag", "arg2", "--help"},
+	}
+	rootCmd.SetHelpFunc(override.helpFunc())
 
 	_, err := executeCommand(rootCmd, "child", "arg1", "--myflag", "arg2", "--help")
 	if err != nil {
-		t.Errorf("Unexpected error: %v", err)
+		t.Errorf("Unexpected error executing command: %v", err)
 	}
 
-	if !helpCalled {
-		t.Error("Overridden help function not called")
+	if err = override.checkError(); err != nil {
+		t.Errorf("Unexpected error from help function: %v", err)
 	}
 }
 
 func TestHelpCommandOverrideOnChild(t *testing.T) {
-	var helpCalled bool
 	rootCmd := &Command{Use: "root"}
 	subCmd := &Command{Use: "child"}
 	rootCmd.AddCommand(subCmd)
 
-	subCmd.SetHelpFunc(func(c *Command, args []string) {
-		helpCalled = true
-		if c.Name() != "child" {
-			t.Errorf(`Expected command name: "child", got %q`, c.Name())
-		}
-		if len(args) != 2 || args[0] != "arg1" || args[1] != "arg2" {
-			t.Errorf("Expected args [args1 arg2], got %v", args)
-		}
-	})
+	override := overridingHelp{
+		expectedCmdName: subCmd.Name(),
+		expectedArgs:    []string{"arg1", "arg2"},
+	}
+	subCmd.SetHelpFunc(override.helpFunc())
 
 	_, err := executeCommand(rootCmd, "help", "child", "arg1", "arg2")
 	if err != nil {
-		t.Errorf("Unexpected error: %v", err)
+		t.Errorf("Unexpected error executing command: %v", err)
 	}
 
-	if !helpCalled {
-		t.Error("Overridden help function not called")
+	if err = override.checkError(); err != nil {
+		t.Errorf("Unexpected error from help function: %v", err)
 	}
 }
 
 func TestHelpCommandOverrideOnRootWithChild(t *testing.T) {
-	var helpCalled bool
 	rootCmd := &Command{Use: "root"}
 	subCmd := &Command{Use: "child"}
 	rootCmd.AddCommand(subCmd)
 
-	rootCmd.SetHelpFunc(func(c *Command, args []string) {
-		helpCalled = true
-		if c.Name() != "child" {
-			t.Errorf(`Expected command name: "child", got %q`, c.Name())
-		}
-		if len(args) != 2 || args[0] != "arg1" || args[1] != "arg2" {
-			t.Errorf("Expected args [args1 arg2], got %v", args)
-		}
-	})
+	override := overridingHelp{
+		expectedCmdName: subCmd.Name(),
+		expectedArgs:    []string{"arg1", "arg2"},
+	}
+	rootCmd.SetHelpFunc(override.helpFunc())
 
 	_, err := executeCommand(rootCmd, "help", "child", "arg1", "arg2")
 	if err != nil {
-		t.Errorf("Unexpected error: %v", err)
+		t.Errorf("Unexpected error executing command: %v", err)
 	}
 
-	if !helpCalled {
-		t.Error("Overridden help function not called")
+	if err = override.checkError(); err != nil {
+		t.Errorf("Unexpected error from help function: %v", err)
 	}
 }