Pass arguments to the help function by marckhouzam · Pull Request #2158 · spf13/cobra

marckhouzam · 2024-06-10T01:46:07Z

This PR makes sure that when Cobra calls HelpFunc()(cmd, args), the proper arguments are passed to the function.
With this, when the help function is overridden, the new function can choose to print the help output based on what the args are.

For example, say I write my own ls program, I could override the help function using SetHelpFunc() so that

ls -h would print List the content of the current directory
ls mydir -h would instead print List the content of the "mydir" directory

This PR fixed the two cases where the help function is called:

when using the --help/-h flag (e.g., prog sub arg1 -h)
when using the help command (e.g., prog help sub arg1)

Another value of this fix is for programs that use plugins and want to ask the plugin what the help output should be.
For instance the tanzu CLI does this, where if I type tanzu help cluster list the help function will call the cluster plugin but needs to tell it that it needs the help for the list command. This list argument needs this PR to get passed to the help function. One can see how the tanzu code had to use os.Args to work around this bug.

Unit tests have been added which also illustrate the different cases that that PR fixes.
A test program is provided in #2154 to show the problem before this PR.

Please refer to a couple of comments I will add in the PR review for a concern about backwards-compatibility.

Signed-off-by: Marc Khouzam <[email protected]>

github-actions · 2024-06-10T01:46:22Z

This PR exceeds the recommended size of 200 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR might be rejected due to its size.

marckhouzam · 2024-06-10T01:49:59Z

command.go

 		// Always show help if requested, even if SilenceErrors is in
 		// effect
 		if errors.Is(err, flag.ErrHelp) {
-			cmd.HelpFunc()(cmd, args)


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.

marckhouzam · 2024-06-10T01:58:33Z

command.go

+			if cmd.DisableFlagParsing {
+				argWoFlags = flags
+			}
+			cmd.HelpFunc()(cmd, argWoFlags)


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?

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

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.

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

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.

marckhouzam · 2024-06-10T01:59:21Z

command.go

 					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)


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

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 }

marckhouzam · 2024-06-10T02:01:54Z

@jpmcb Could you have a look at this? It is 7 lines of actual code with the rest being tests.

@nirs I've seen recently how you are providing some great help to the Cobra project and your input here would be appreciated.

command.go

nirs · 2024-06-10T12:14:42Z

command.go

+			if cmd.DisableFlagParsing {
+				argWoFlags = flags
+			}
+			cmd.HelpFunc()(cmd, argWoFlags)


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

command_test.go

nirs · 2024-06-10T13:11:30Z

command_test.go

+		if len(args) != 2 || args[0] != "arg1" || args[1] != "arg2" {
+			t.Errorf("Expected args [args1 arg2], got %v", args)
+		}
+	})


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 { ... }

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

nirs · 2024-06-11T12:01:22Z

This PR fixed the two cases where the help function is called:

when using the --help/-h flag (e.g., prog sub arg1 -h)

when using the help command (e.g., prog help sub arg1)

I think this change is not needed and we can keep the current behavior, and fix the
case of the more common and useful -h/--help.

Another value of this fix is for programs that use plugins and want to ask the plugin what the help output should be. For instance the tanzu CLI does this, where if I type tanzu help cluster list the help function will call the cluster plugin but needs to tell it that it needs the help for the list command. This list argument needs this PR to get passed to the help function.

Why would someone type:

tanzu help cluster list

and then type work hard to remove the help command to run the actual command, instead of adding -h to the actual command:

tanzu cluster list -h

and finally remove the -h for running the actual command?

I guess tanszu smart help works both for help and -h/--help, so if we fix -h,--help, we can remove the workarounds?

It we spend time on the help command, we should make it easy to show different content for "help command". For example try git log -h and git help log. This is the biggest problems with Go tools - the online help is not good enough and we never have manual pages.

github-actions · 2024-06-11T15:07:27Z

This PR exceeds the recommended size of 200 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR might be rejected due to its size.

Signed-off-by: Marc Khouzam <[email protected]>

github-actions · 2024-06-11T15:09:42Z

This PR exceeds the recommended size of 200 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR might be rejected due to its size.

marckhouzam · 2024-06-11T15:17:04Z

This PR fixed the two cases where the help function is called:

when using the --help/-h flag (e.g., prog sub arg1 -h)

when using the help command (e.g., prog help sub arg1)

I think this change is not needed and we can keep the current behavior, and fix the case of the more common and useful -h/--help.

Answered here: #2158 (comment)

Another value of this fix is for programs that use plugins and want to ask the plugin what the help output should be. For instance the tanzu CLI does this, where if I type tanzu help cluster list the help function will call the cluster plugin but needs to tell it that it needs the help for the list command. This list argument needs this PR to get passed to the help function.

Why would someone type:
tanzu help cluster list
and then type work hard to remove the help command [...]

I agree that the help command is not very useful and I personally never use it.
However, I still have to make it work for others who may use it. 😄

I guess tanzu smart help works both for help and -h/--help, so if we fix -h,--help, we can remove the workarounds?

The same override of HelpFunc() does work for both help and -h/--help but that is because Cobra uses the same HelpFunc() in both cases. So we need to fix it for both cases.

It we spend time on the help command, we should make it easy to show different content for "help command". For example try git log -h and git help log. This is the biggest problems with Go tools - the online help is not good enough and we never have manual pages.

This sounds like a good idea. Programs using Cobra can do this already by overriding the help command completely, and provide something different between help and -h. But maybe we can make it easier for them so that we can promote the idea of having different output in both cases. I like the example of git help log vs git log -h that you provide. It would be great if Cobra could guide projects to do something like that.

nirs · 2024-06-11T18:24:44Z

command_test.go

+				o.err = fmt.Errorf("Expected args %v, got %v", o.expectedArgs, args)
+				return
+			}
+		}


We can extract lines 1098 to 1108 to a argsEqual() helper that can be useful in other tests, and can be replaced with slices.Equal when we can required Go 1.18. Can also be a good place to document why we cannot use slices.Equal.

Would be little bit nicer since since we don't have duplicate the error here:

if !argsEqual(args, o,expectedArgs) { o.err = fmt.Errorf("Expected args %v, got %v", o.expectedArgs, args) }

nirs · 2024-06-11T18:28:39Z

command_test.go

+}
+
+func (o *overridingHelp) helpFunc() func(c *Command, args []string) {
+	return func(c *Command, args []string) {


Any reason for returning a help function instead of implementing one?

This could be:

func (o *overridingHelp) helpFunc(c *Command, args []string) { ... }

And used later as:

rootCmd.SetHelpFunc(override.helpFunc)

nirs · 2024-06-11T18:39:41Z

command_test.go

+	}
+	rootCmd.SetHelpFunc(override.helpFunc())
+
+	_, err := executeCommand(rootCmd, "child", "arg1", "--myflag", "arg2", "--help")


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

nirs · 2024-06-11T18:41:59Z

command_test.go

+	if err = override.checkError(); err != nil {
+		t.Errorf("Unexpected error from help function: %v", err)
+	}
+}


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?

nirs · 2024-06-11T18:59:41Z

command.go

+				// was not done and cmd.Flags().Args() is not filled.  We therefore
+				// use the full set of arguments, which include flags.
+				remainingArgs = flags
+			}


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

nirs · 2024-06-11T19:47:40Z

command.go

+			if cmd.DisableFlagParsing {
+				argWoFlags = flags
+			}
+			cmd.HelpFunc()(cmd, argWoFlags)


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.

nirs · 2024-06-11T19:59:32Z

command.go

 					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)


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 }

Pass arguments to the help function

8e51700

Signed-off-by: Marc Khouzam <[email protected]>

marckhouzam commented Jun 10, 2024

View reviewed changes

nirs reviewed Jun 10, 2024

View reviewed changes

Address comments

9fbba57

Signed-off-by: Marc Khouzam <[email protected]>

marckhouzam force-pushed the fix/helpFuncArgs branch from bec76b4 to 9fbba57 Compare June 11, 2024 15:09

nirs reviewed Jun 11, 2024

View reviewed changes

Conversation

marckhouzam commented Jun 10, 2024

Uh oh!

github-actions bot commented Jun 10, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

-h, --help use case:

help use case

Uh oh!

marckhouzam Jun 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nirs Jun 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

marckhouzam commented Jun 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

-h, --help use case:

help use case

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nirs commented Jun 11, 2024

Uh oh!

github-actions bot commented Jun 11, 2024

Uh oh!

github-actions bot commented Jun 11, 2024

Uh oh!

marckhouzam commented Jun 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nirs Jun 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nirs Jun 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

marckhouzam Jun 11, 2024 •

edited

Loading

nirs Jun 11, 2024 •

edited

Loading

marckhouzam commented Jun 10, 2024 •

edited

Loading

marckhouzam commented Jun 11, 2024 •

edited

Loading

nirs Jun 11, 2024 •

edited

Loading

nirs Jun 11, 2024 •

edited

Loading