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

Pass arguments to the help function #2158

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

Pass arguments to the help function #2158

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
8e51700
Pass arguments to the help function
marckhouzam Jun 1, 2024
9fbba57
Address comments
marckhouzam Jun 11, 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
19 changes: 14 additions & 5 deletions command.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -470,8 +470,11 @@ func (c *Command) HelpFunc() func(*Command, []string) {
}

// Help puts out the help for the command.
// Used when a user calls help [command].
// Can be defined by user by overriding HelpFunc.
// Kept for backwards compatibility.
marckhouzam marked this conversation as resolved.
Show resolved Hide resolved
// 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 {
c.HelpFunc()(c, []string{})
return nil
Expand Down Expand Up @@ -1119,7 +1122,13 @@ func (c *Command) ExecuteC() (cmd *Command, err error) {
// Always show help if requested, even if SilenceErrors is in
// effect
if errors.Is(err, flag.ErrHelp) {
cmd.HelpFunc()(cmd, args)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The use of args here was added in this commit and it does seem that it was a mistake; I believe the flags variable should have been used instead.

// The call to execute() above has parsed the flags.
// We therefore only pass the remaining arguments to the help function.
argWoFlags := cmd.Flags().Args()
marckhouzam marked this conversation as resolved.
Show resolved Hide resolved
if cmd.DisableFlagParsing {
argWoFlags = flags
}
marckhouzam marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Contributor

@nirs nirs Jun 11, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Thanks for explaining and adding the comment.

I think this will communicates better the intent of the code:

// The call to execute() above has parsed the flags.
// We therefore only pass the remaining arguments to the help function.
var remainingArgs []string
if cmd.DisableFlagParsing {
    // For commands that have DisableFlagParsing == true, the flag parsing was
    // not done, therefore use the full set of arguments, which include flags.
    remainingArgs = flags
} else {
    remainingArgs = cmd.Flags().Args()

}

cmd.HelpFunc()(cmd, argWoFlags)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I wanted to discuss backwards-compatibility for this change.
Before this PR cobra would pass all the arguments on the command-line (which was incorrect).
With this PR cobra will pass only the arguments after removing the sub-command and flags, as it does when calling the *Run/RunE functions.

It is possible that some programs worked around this bug and fixing it may affect them.

However, considering that before this PR the help function sometime received all the args (when using the --help flag) but no args at all when called from the help command, it makes it less likely that a workaround used the actual args parameter.

So, I feel it is ok to make this change but if we do maybe we should include a note in the release notes.

@jpmcb how do you feel about this?

Copy link
Contributor

Choose a reason for hiding this comment

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

Sending only the remaining arguments is better when using -h. If we think this may break someone, we can do this only if a new flag is set like HelpGetRemainingArgs so users can opt-int to use the new behavior.

When using help command I'm not sure arguments make sense. You use this to learn about a command before you use it. You use -h,--help to get help for a command while trying to use it. You expect that adding -h,--help anywhere in the current command will show the help and getting more specific help for the current argument is nice improvement. Personally I never use help command since -h is much easier to type.

-h, --help use case:

Trying to run a command:

$ foo bar --baz
error: ...

It did not work, add -h:

$ foo bar --baz -h
help on foo bar...

Use it correctly:

$ foo bar --baz missing-value
success

help use case

What is the foo bar command?

$ foo help bar
help on foo bar

Copy link
Collaborator Author

@marckhouzam marckhouzam Jun 11, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Your use case description is correct. However, there is another (obscure) situation, which is actually the one I'm really trying to fix 😄.

The tanzu CLI uses plugins to provide many of its sub-commands. So a user may do:
tanzu help cluster list
to get the help on the cluster list command.
However, the tanzu CLI only has cluster as a sub-command and needs to call that "cluster" plugin to ask it for the help for the cluster list command. Therefore, the arguments passed to the original tanzu help command are required to know which sub-command of the plugin the user is asking about.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Currently, the tanzu CLI code has to use os.Args to get that info: https://github.com/vmware-tanzu/tanzu-cli/blob/6a11ce93cd4e811e213e8439e090e1d73a053fd3/pkg/cli/plugin_cmd.go#L192-L201

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for explaning this, so this is needed when a command loads its sub commands dynamically. The help commands may need to do the same dynamic loading to display the help.

return cmd, nil
}

Expand Down Expand Up @@ -1260,14 +1269,14 @@ Simply type ` + c.displayName() + ` help [path to command] for full details.`,
return completions, ShellCompDirectiveNoFileComp
},
Run: func(c *Command, args []string) {
cmd, _, e := c.Root().Find(args)
cmd, remainingArgs, e := c.Root().Find(args)
if cmd == nil || e != nil {
c.Printf("Unknown help topic %#q\n", args)
CheckErr(c.Root().Usage())
} else {
cmd.InitDefaultHelpFlag() // make possible 'help' flag to be shown
cmd.InitDefaultVersionFlag() // make possible 'version' flag to be shown
CheckErr(cmd.Help())
cmd.HelpFunc()(cmd, remainingArgs)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I don't feel this has any backwards-compatibility implications since the args value was always empty before this change for this case.

Copy link
Contributor

@nirs nirs Jun 11, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I think we are missing documentation explaning that Run, RunE, and HelpFunc accept the remaining arguments after parsing. Previously HelpFunc sometimes accepted no arguments and sometimes accepted the raw parsed arguments.

Can be here:

// SetHelpFunc sets help function. Can be defined by Application.
// The help function is called with the remaining arguments after parsing
// the flags. If flag parsing is disabled it is called with all arguments.
func (c *Command) SetHelpFunc(f func(*Command, []string)) {
	c.helpFunc = f
}

}
},
GroupID: c.helpCommandGroupID,
Expand Down
187 changes: 187 additions & 0 deletions command_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
// See the License for the specific language governing permissions and
// limitations under the License.

//nolint:goconst
package cobra

import (
Expand Down Expand Up @@ -1079,6 +1080,192 @@ func TestHelpExecutedOnNonRunnableChild(t *testing.T) {
checkStringContains(t, output, childCmd.Long)
}

func TestHelpOverrideOnRoot(t *testing.T) {
marckhouzam marked this conversation as resolved.
Show resolved Hide resolved
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())
}
if len(args) != 2 || args[0] != "arg1" || args[1] != "arg2" {
t.Errorf("Expected args [args1 arg2], got %v", args)
marckhouzam marked this conversation as resolved.
Show resolved Hide resolved
}
})

_, err := executeCommand(rootCmd, "arg1", "arg2", "--help")
if err != nil {
t.Errorf("Unexpected error: %v", err)
}

if !helpCalled {
t.Error("Overridden help function not called")
}
}

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

_, err := executeCommand(rootCmd, "child", "arg1", "arg2", "--help")
if err != nil {
t.Errorf("Unexpected error: %v", err)
}

if !helpCalled {
t.Error("Overridden help function not called")
}
}

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

_, err := executeCommand(rootCmd, "child", "arg1", "arg2", "--help")
if err != nil {
t.Errorf("Unexpected error: %v", err)
}

if !helpCalled {
t.Error("Overridden help function not called")
}
}

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

_, err := executeCommand(rootCmd, "child", "arg1", "--myflag", "arg2", "--help")
Copy link
Contributor

Choose a reason for hiding this comment

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

Why test additional flags separately? this way we don't cover all cases.

if err != nil {
t.Errorf("Unexpected error: %v", err)
}

if !helpCalled {
t.Error("Overridden help function not called")
}
}

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

_, err := executeCommand(rootCmd, "child", "arg1", "--myflag", "arg2", "--help")
if err != nil {
t.Errorf("Unexpected error: %v", err)
}

if !helpCalled {
t.Error("Overridden help function not called")
}
}

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

_, err := executeCommand(rootCmd, "help", "child", "arg1", "arg2")
if err != nil {
t.Errorf("Unexpected error: %v", err)
}

if !helpCalled {
t.Error("Overridden help function not called")
}
}

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

Choose a reason for hiding this comment

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

We repeat the code of the help function many times, maybe add a helper struct keeping the , so we can do:

 h := helper{t: t, command: "child", args: []string{"args1", "arg2"}}
 rootCmd.SetHelpFunc(h.helpFunc)
 _, err := executeCommand(rootCmd, "help", "child", "arg1", "arg2")

 if !h.helpCalled {
     ...
 }

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I like it!
I tried to do what you suggest.


_, err := executeCommand(rootCmd, "help", "child", "arg1", "arg2")
if err != nil {
t.Errorf("Unexpected error: %v", err)
}

if !helpCalled {
t.Error("Overridden help function not called")
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

In both help command you don't test additional flags - if you want to have the same behavior for help command, would it be good to ensure that parsed flags are not passed to the help function?


func TestVersionFlagExecuted(t *testing.T) {
rootCmd := &Command{Use: "root", Version: "1.0.0", Run: emptyRun}

Expand Down