add PersistentPreRun to disable required flag for help and completion command

[JunNishimura]

Overview

Fixes #1918.

The auto-implemented subcommands (help and completion) did't work when MarkPersistentRequiredFlag() called on root command. So I set DisableFlagParsing true for HelpCommand and CompletionCommand (bash, zsh, fish, powershell) to avoid required flags validation.

I also added unit test to see if the code works as I expected.

Related Issue

• https://github.com/spf13/cobra/issues/1918

[CLAassistant]

All committers have signed the CLA.

[marckhouzam]

Thanks @JunNishimura.

However this change causes a couple of regressions (that are apparently not tested).

1. ./prog completion bash --no-descriptions no longer works, and the same for the other three shells
2. ./prog completion bash -h no longer works, and the same for the other three shells
3. ./prog help -h no longer works (it used to show a help for the help itself)

I don't think we can disable flag parsing on those commands. We have to find another approach.

You may be able to add a PersistentPreRun function to those two commands which would disable the annotation that marks a flag as required. Something like:

			PersistentPreRun: func(cmd *Command, args []string) {
				cmd.Flags().VisitAll(func(pflag *flag.Flag) {
					requiredAnnotation, found := pflag.Annotations[BashCompOneRequiredFlag]
					if found && requiredAnnotation[0] == "true" {
                                                  // Disable any persistent required flags for the help command
						pflag.Annotations[BashCompOneRequiredFlag] = []string{"false"}
					}
				})
			},

[JunNishimura]

Thanks, @marckhouzam

Your point was right. I didn't notice that my previous code caused some regressions.

And, I appreciate the alternative suggestions. I implemented your suggestions in the code.

I would appreciate it if you check the commits again.

[marckhouzam]

@JunNishimura This looks pretty good. There is a subtle backwards-compatibility impact that we should probably protect against. If you look at where the PersistentPreRun functions are called you will notice that if a command has a PersistentPreRun, the PersistentPreRun of its parent is not called: https://github.com/spf13/cobra/blob/dcb405a9399ed307eaa0e50f5cb1c290c8979dd4/command.go#L913-L923)

So here is the scenario:

1. a CLI tool using Cobra has defined a PersistentPreRun function on its root command which does something that should be done for every command (e.g., collect some statistics about which command is being called)
2. this PR adds a PersistentPreRun on the sub-commands completion and help

Because of the new PersistentPreRun on completion and help, the root.PersistentPreRun will no longer be called for those two sub-commands. This is a change in behaviour and could affect projects.

I suggest that at the end of the two new PersistentPreRun functions, we actually call root.PersistentPreRun to get the original behaviour back.

Does that make sense?

[JunNishimura]

Thanks, @marckhouzam

I understand the situation. I'm so glad to know the algorithm of cobra thanks to your points.

I implemented to call root's PersistentPreRun in sub-command's (help and completion) PersistentPreRun. I also added unit tests to the above implementation just in case.

I would appreciate it if you check the commits again.

[marckhouzam]

@JunNishimura The backwards-compatibility fix is a little more complicated. For example the root.PersistentPreRun could be nil, so we cannot blindly call it. Also, the root command may not use PersistentPreRun but instead PersistentPreRunE (notice the final E on the second one). So we should mimic what cobra does already to make this work properly. I'm thinking of something like this:

  if cmd.Root().PersistentPreRunE != nil {
      return cmd.Root().PersistentPreRunE(cmd, args)
  } else if cmd.Root().PersistentPreRun != nil {
      cmd.Root().PersistentPreRun(cmd, args)
  }
  return nil

To be able to propagate any error returned by cmd.Root().PersistentPreRunE you will probably have to use PersistentPreRunE for the help and completion commands.

Let me know if you need any clarifications, and thanks again for your efforts!

[JunNishimura]

Thanks, @marckhouzam

Your point is right. My implementation did not take into account the possibility that root.PersistentPreRun is nil.

I modified to use PersitentPreRunE to be able to propagate any error returned by root.PersistentPreRunE instaed of PersistentPreRun. I also added a check to see if the function is not nil.

Sorry for repeating myself, but I'd appreciate it if you'd check commits again.

[marckhouzam]

@JunNishimura The linter is failing 😢 . It is just that this PR introduce a third string "true" which the linter wants us to define a constant for. Could you please do that and use it in the three instance of "true"

Thanks

[JunNishimura]

Thanks, @marckhouzam

I define a constant for "true" named trueString. The name trueString might be too direct, so if you have a better name, please let me know and I will change it.

[marckhouzam]

Thanks for the quick turnaround with the linter @JunNishimura . This looks good now.

As this change had backwards-compatibility implications

I'll wait a little while to allow @jpmcb to have a look if he wants.

[jpmcb]

Planning to take a look this weekend. Thanks @marckhouzam for the heads up!

[JunNishimura]

@jpmcb Hi! How about this PR?

If you haven't seen it because you are busy or for some reasons, that's perfectly fine, as I do not want to rush you. I would appreciate it if you could take a look when you have time.

I reminded you because I thought it was possible that you simply forgot.

[marckhouzam]

This needs a rebase

[saltbo]

Any update？

[JunNishimura]

@marckhouzam @jpmcb Sorry for the late reply. I resolved the conflict.

I would appreciate it if you check this PR again!!