Support subcommands checking for suggestions

[YuviGold]

Resolves #636
Resolves #834
Resolves alexellis/arkade#525

Till now, only the root command looked for suggestions.
Now findSuggestions works for nested commands as well.

For example

Error: unknown command "rewin" for "root times"

Did you mean this?
	rewind

Run 'root times --help' for usage.

[CLAassistant]

All committers have signed the CLA.

[YuviGold]

/cc @jharshman
/cc @jpmcb

[spf13]

Looks good to me. Would be a nice addition to the #1496

[spf13]

LGTM

[umarcor]

Now listed in #1496 and merged in #1525.

@spf13, maybe you can merge some of the PRs listed in #1496? Or, alternatively, provide merging permissions to some active contributor such as @marckhouzam.

[marckhouzam]

I am concerned that this change is not backwards-compatible and will break a bunch of programs.

Before this PR, commands (except the root command) that don't specify the Args field would accept both sub-commands and arguments. With this PR, only sub-commands will be accepted. For example, if I have a program that has one sub-command and one sub-sub-command like so (see full program at the end of this comment):

$ prog sub subSub

Before this PR:

$ ./prog sub somearg
sub called with args [somearg]

but with this PR:

$ ./prog sub somearg
Error: unknown command "somearg" for "prog sub"
Run 'prog sub --help' for usage.

I believe this is a valid scenario that we will find in the wild. I know that the helm project used to take avantage of this possibility (it no longer does to improve its shell completion usage). For example, using an old version of helm:

$ helm2 get -h
[...]
Usage:
  helm get [flags] RELEASE_NAME
  helm get [command]

Available Commands:
  hooks       Download all hooks for a named release
  manifest    Download the manifest for a named release
  notes       Displays the notes of the named release
  values      Download the values file for a named release
[...]

Notice how the helm2 get command would accept a release name as an argument, or any of the allowed sub-commands. This would break with this PR.

Test program used in the above example:

package main

import (
	"fmt"

	"github.com/spf13/cobra"
)

var root = &cobra.Command{
	Use: "prog",
	Run: func(cmd *cobra.Command, args []string) {
		fmt.Printf("prod called with args %v\n", args)
	},
}

var sub = &cobra.Command{
	Use: "sub",
	Run: func(cmd *cobra.Command, args []string) {
		fmt.Printf("sub called with args %v\n", args)
	},
}

var subSub = &cobra.Command{
	Use: "subSub",
	Run: func(cmd *cobra.Command, args []string) {
		fmt.Printf("subSub called with args %v\n", args)
	},
}

func main() {
	root.AddCommand(sub)
	sub.AddCommand(subSub)

	root.Execute()
}

[umarcor]

@marckhouzam, I agree that this PR introduces a change that might break a bunch of programs. However, my understanding is that those programs are relaying on "undefined behaviour"; hence, it is not a breaking change from a semver perspective.

See https://github.com/spf13/cobra/blob/master/user_guide.md#positional-and-custom-arguments. There is no documented default behaviour if Args is not specified. helm and other projects might be expecting the default to be ArbitraryArgs. That is the implicit behaviour of the codebase indeed:

cobra/command.go

Lines 998 to 1003 in 9e1d6f1

	func (c *Command) ValidateArgs(args []string) error {
	if c.Args == nil {
	return nil
	}
	return c.Args(c, args)
	}

cobra/args.go

Lines 55 to 57 in 9e1d6f1

	func ArbitraryArgs(cmd *Command, args []string) error {
	return nil
	}

In order to assume that defaulting to ArbitraryArgs is explicit and not arbitrary (unspecified), we should change L999-L1001 of command.go to:

	if c.Args == nil {
		return ArbitraryArgs(c, args)
	}

and update the documentation accordingly. I believe that might be done on top of this PR.

On the other hand, as far as I understand, @YuviGold is changing the legacy behaviour. That was apparently inconsistent with regard to the root command and subcommands, and he is making it consistent. It was surprising for me, because I would expect the "legacy" to be kept untouched, then deprecated, last removed; never updated. Moreover, I wonder if this update wouldn't be equivalent to actually removing the usage of legacyArgs from the codebase. Anyway, since this PR was explicitly reviewed by @spf13, and he requested it to be included in the next release, I guess it is ok. I'd expect @spf13 to be the one who best understands the rationale behind legacyArgs.

I will move this PR to the end of #1525, so we can keep discussing about it while other PRs are merged.

[YuviGold]

Thank you @marckhouzam .I honestly didn't think about this use case.
Although as @umarcor pointed out, this seems to be the expected behavior.
So please, let me know what do you decide and if there's something needed to be done. :)

I believe that this multi-level suggestion is useful and necessary for many tools.
For example, cli/cli implemented the suggestions themselves while I would expect cobra to support it.
https://github.com/cli/cli/blob/029d49f3b38ebd54cd84b23732bb7efabfce4896/pkg/cmd/root/help.go#L51-L76

// Display helpful error message in case subcommand name was mistyped.
// This matches Cobra's behavior for root command, which Cobra
// confusingly doesn't apply to nested commands

[marckhouzam]

@umarcor

Anyway, since this PR was explicitly reviewed by @spf13, and he requested it to be included in the next release, I guess it is ok. I'd expect @spf13 to be the one who best understands the rationale behind legacyArgs.

I of course agree that @spf13 and the maintainers have the final word on this. But I think it would be quite understandable if @spf13 did not realize that this was a breaking change. Considering the stated importance of very strong stability for Cobra, my guess is that this change should be avoided.

I will move this PR to the end of #1525, so we can keep discussing about it while other PRs are merged.

Thanks!

@YuviGold

I believe that this multi-level suggestion is useful and necessary for many tools.

I agree. Maybe a backwards-compatible approach can be defined? At a minimum an opt-in solution would be backwards-compatible, but there may be something even better; I haven't looked into it myself.

[umarcor]

I of course agree that @spf13 and the maintainers have the final word on this. But I think it would be quite understandable if @spf13 did not realize that this was a breaking change. Considering the stated importance of very strong stability for Cobra, my guess is that this change should be avoided.

@marckhouzam, I suggest you or anyone else to propose a PR which adds a test that acknowledges the current de facto behaviour that other tools might be relying on. It seems that legacyArgs is not properly tested. If we merge that one before this PR, then @YuviGold will need to account for those use cases so that CI is green here.

BTW, see the updated tests in #841. Note also: "If Args is undefined or nil, it defaults to ArbitraryArgs".

@YuviGold, I think you, cli/cli and other projects are exploiting an unwanted feature in the codebase. If you want to "support subcommands checking for suggestions" (which is the title of this PR), my understanding is you might achieve it by setting Args properly. You should not need to modify legacyArgs at all.

Overall, projects relying on both the "previous" behaviour and on the "after" behaviour are doing it wrong by relying on legacyArgs. They should not care about this PR, but use the library appropriately instead of driving corner cases to their flavours.

[spf13]

I hadn't considered the breaking change this would introduce.

I disagree that changing undocumented behavior is ok. The code implicitly is the documentation. It's hard to say someone has done something wrong when it worked for them.

I agree with @marckhouzam

Maybe a backwards-compatible approach can be defined? At a minimum an opt-in solution would be backwards-compatible, but there may be something even better; I haven't looked into it myself.

I think we should consider how large the impact this is. If Helm is expecting the current behavior still and others are as well we can take 3 approaches.

1. Make this functionality opt in.
2. Make this the default functionality but provide an opt in for folks wanting to use the legacy behavior (a simple path to working with the fix in place)
3. Wait a release, put some notice / warning in this release that this will change in the next release and since it's a bugfix, it's not a "new version" change.

[umarcor]

FTR, I removed this PR from #1525 and I moved it to section "Delayed to a later release" in #1496.

[jpmcb]

I agree with Steve - this would be an unexpected breaking change for users.

I'm ok putting a "notice" in and upcoming release of breaking changes upcoming in a 2.0.0 cobra release. I'm not a huge fan of letting lingering tech debt exist next to new implementation (a.k.a, the opt in / opt out approach). We've already done this with the shell completions opt in / opt out and it's a bit painful to maintain.

[github-actions]

This PR is being marked as stale due to a long period of inactivity

[jpmcb]

Hi @YuviGold - I think this is still valid (and worth discussing). Can you recreate this against the main branch since master has been deleted?