Misbehaviour of IsHelpCommand()

[n10v]

I notice, what IsHelpCommand always returns false even on built-in help command and I found out why:
Look at the docs of IsHelpCommand:

IsHelpCommand determines if a command is a 'help' command; a help command is
determined by the fact that it is NOT runnable/hidden/deprecated, and has no
sub commands that are runnable/hidden/deprecated.

In the docs it says, what help is not runnable, but if you look in initHelpCmd definition

func (c *Command) initHelpCmd() {
	if c.helpCommand == nil {
		if !c.HasSubCommands() {
			return
		}

		c.helpCommand = &Command{
			Use:   "help [command]",
			Short: "Help about any command",
			Long: `Help provides help for any command in the application.
    Simply type ` + c.Name() + ` help [path to command] for full details.`,
			PersistentPreRun:  func(cmd *Command, args []string) {},
			PersistentPostRun: func(cmd *Command, args []string) {},

			Run: func(c *Command, args []string) {
				cmd, _, e := c.Root().Find(args)
				if cmd == nil || e != nil {
					c.Printf("Unknown help topic %#q.", args)
					c.Root().Usage()
				} else {
					cmd.Help()
				}
			},
		}
	}
	c.AddCommand(c.helpCommand)
}

, you can see, what built-in help command has Run function.
So the actual logic of IsHelpCommand is wrong. I think, the possible solution could be:

func (c *Command) IsHelpCommand() bool {
    helpCmd := c.findHelpCommand()
    if c == helpCmd{
        return true
    }
    return false
}

func (c Command) findHelpCommand() *Command {
    if c.helpCommand == nil {
        if c.HasParent() {
             return c.Parent().findHelpCommand()
        }
    }
    return c.helpCommand
}

[n10v]

Continue of #394 (comment)
@eparis I also can be not right, but it should be fixed anyway.

[eparis]

I agree this is terribly confusing... People have used these 'help commands' as a place to put additional help information but on this which were not runningable. So you might have a cobra command tree like the following

rootCmd
|-rootExtraHelpCmd
|-subCmd
  |-subCmdExtraHelpCmd

Where rootCmd and subCmd have Run() functions, but rootExtraHelpCmd and subCmdExtraHelpCmd merely exist as a place to do rootCmd rootExtraHelpCmd --help and get more information. These types of subcommands are used by some projects.

Now if the actual "help" command fits this pattern or not could be a question. Maybe it should be part of the IsHelpCommand() even though it is runnable. So it would be a special case. Would need to make sure it shows up in the right places in the right help outputs...

[n10v]

Can you please show a concrete example, because unfortunately I don't fully understand you😰

[eparis]

Ignore the name 'IsHelpCommand()'. It is a terrible name. The correct name should have be IsAdditionalHelpTopicCommand(). A subcommand which can't actually run and serves no purpose other than to provide addition help topic information.

Say my program is something that can write "stuff". It can write to a 'URL' or a 'Word Doc'. I might create the program with the Use: line like:

Use: "write [URL|word]"

I might create a URLSubcmd and a WodSubcmd. Where neither was runnable but the Long: section of each gave a lot more information about a URL or a Word Doc respectively.

If I ran write --help it would not show any subcmds but would show URL and Word as 'additional help topics'. So I would be able to do write URL --help and get help about URLs. Or write Word --help and get help about Word documents.

Does that make sense? Ignore the bad name 'IsHelpSubCommand()' and think of the name 'IsAdditionalHelpTopicCommand()`

[n10v]

Yes, now I understand, what did you mean. And yes, name and docs of IsHelpCommand is really confusing and are wrong at all.
But as I understand, it should not be renamed because of back compatibility.

[n10v]

So how this situation should be handled?
I think, at first the docs of IsHelpCommand should be changed and it should says, what this function really does.
Should we define new function to really detect if command is help? (It would be useful for fixing #366)

[eparis]

Actually I wouldn't mind renaming it. It is an API break, but I highly doubt we'll hit many people. If we want to play it safe make the comment say it is deprecated and have it just call the function with a better name. It was only ever made public so it could be used in the template.

[n10v]

I also don't mind to rename it. And deprecating this function will be better.