From d0bb3e33e6a2daf86a1c7809e6558656052555af Mon Sep 17 00:00:00 2001 From: Eric Paris Date: Tue, 9 Jun 2015 13:52:43 -0400 Subject: [PATCH 1/5] rework Find() to make it more obvious what is happening We had lots of quirky if statements like `commandFound.Name() == c.Name() && len(stripFlags(args, c)) > 0 && commandFound.Name() != args[0]` which embeed all sorts of artifacts which are hard to parse. So in general, just try to simplify and make stuff readable. --- command.go | 58 ++++++++++++++++++++++++++---------------------------- 1 file changed, 28 insertions(+), 30 deletions(-) diff --git a/command.go b/command.go index 141cfab..4209c8d 100644 --- a/command.go +++ b/command.go @@ -381,49 +381,47 @@ func (c *Command) Find(args []string) (*Command, []string, error) { return nil, nil, fmt.Errorf("Called find() on a nil Command") } - // If there are no arguments, return the root command. If the root has no - // subcommands, args reflects arguments that should actually be passed to - // the root command, so also return the root command. - if len(args) == 0 || !c.Root().HasSubCommands() { - return c.Root(), args, nil - } - var innerfind func(*Command, []string) (*Command, []string) innerfind = func(c *Command, innerArgs []string) (*Command, []string) { - if len(innerArgs) > 0 && c.HasSubCommands() { - argsWOflags := stripFlags(innerArgs, c) - if len(argsWOflags) > 0 { - matches := make([]*Command, 0) - for _, cmd := range c.commands { - if cmd.Name() == argsWOflags[0] || cmd.HasAlias(argsWOflags[0]) { // exact name or alias match - return innerfind(cmd, argsMinusFirstX(innerArgs, argsWOflags[0])) - } else if EnablePrefixMatching { - if strings.HasPrefix(cmd.Name(), argsWOflags[0]) { // prefix match - matches = append(matches, cmd) - } - for _, x := range cmd.Aliases { - if strings.HasPrefix(x, argsWOflags[0]) { - matches = append(matches, cmd) - } - } + argsWOflags := stripFlags(innerArgs, c) + if len(argsWOflags) == 0 { + return c, innerArgs + } + nextSubCmd := argsWOflags[0] + matches := make([]*Command, 0) + for _, cmd := range c.commands { + if cmd.Name() == nextSubCmd || cmd.HasAlias(nextSubCmd) { // exact name or alias match + return innerfind(cmd, argsMinusFirstX(innerArgs, nextSubCmd)) + } + if EnablePrefixMatching { + if strings.HasPrefix(cmd.Name(), nextSubCmd) { // prefix match + matches = append(matches, cmd) + } + for _, x := range cmd.Aliases { + if strings.HasPrefix(x, nextSubCmd) { + matches = append(matches, cmd) } } - - // only accept a single prefix match - multiple matches would be ambiguous - if len(matches) == 1 { - return innerfind(matches[0], argsMinusFirstX(innerArgs, argsWOflags[0])) - } } } + // only accept a single prefix match - multiple matches would be ambiguous + if len(matches) == 1 { + return innerfind(matches[0], argsMinusFirstX(innerArgs, argsWOflags[0])) + } + return c, innerArgs } commandFound, a := innerfind(c, args) - - // If we matched on the root, but we asked for a subcommand, return an error argsWOflags := stripFlags(a, commandFound) + + // no subcommand, always take args + if !commandFound.HasSubCommands() { + return commandFound, a, nil + } + // root command with subcommands, do subcommand checking if commandFound == c && len(argsWOflags) > 0 { return nil, a, fmt.Errorf("unknown command %q", argsWOflags[0]) } From 0a1a8e2e92dbcd5a031012169aadca8f3d466723 Mon Sep 17 00:00:00 2001 From: Eric Paris Date: Tue, 9 Jun 2015 15:04:14 -0400 Subject: [PATCH 2/5] Remove (unused) cmdErrorBuf Nothing was using it any more... --- command.go | 13 ------------- 1 file changed, 13 deletions(-) diff --git a/command.go b/command.go index 4209c8d..c9c1967 100644 --- a/command.go +++ b/command.go @@ -84,7 +84,6 @@ type Command struct { commandsMaxNameLen int flagErrorBuf *bytes.Buffer - cmdErrorBuf *bytes.Buffer args []string // actual args parsed from flags output *io.Writer // nil means stderr; use Out() method instead @@ -458,17 +457,7 @@ func (c *Command) execute(a []string) (err error) { return nil } if err != nil { - // We're writing subcommand usage to root command's error buffer to have it displayed to the user - r := c.Root() - if r.cmdErrorBuf == nil { - r.cmdErrorBuf = new(bytes.Buffer) - } - // for writing the usage to the buffer we need to switch the output temporarily - // since Out() returns root output, you also need to revert that on root - out := r.Out() - r.SetOutput(r.cmdErrorBuf) c.Usage() - r.SetOutput(out) return err } // If help is called, regardless of other flags, we print that. @@ -595,8 +584,6 @@ func (c *Command) initHelp() { func (c *Command) ResetCommands() { c.commands = nil c.helpCommand = nil - c.cmdErrorBuf = new(bytes.Buffer) - c.cmdErrorBuf.Reset() } //Commands returns a slice of child commands. From 6f735782e098d0929212170aee549de97731beea Mon Sep 17 00:00:00 2001 From: Eric Paris Date: Tue, 9 Jun 2015 15:16:44 -0400 Subject: [PATCH 3/5] Remove unused ErrHelp check Inside Command.Execute() we were checking for pflag.ErrHelp. But Command.execute() never returns that value. It just complicates the code and isn't used. --- command.go | 9 ++------- 1 file changed, 2 insertions(+), 7 deletions(-) diff --git a/command.go b/command.go index c9c1967..4bcec0c 100644 --- a/command.go +++ b/command.go @@ -549,13 +549,8 @@ func (c *Command) Execute() (err error) { } if err != nil { - if err == flag.ErrHelp { - c.Help() - - } else { - c.Println("Error:", err.Error()) - c.Printf("Run '%v help' for usage.\n", c.Root().Name()) - } + c.Println("Error:", err.Error()) + c.Printf("Run '%v help' for usage.\n", c.Root().Name()) } return From 0a7a8500261c7d1261c022a6d74e8d70166addf7 Mon Sep 17 00:00:00 2001 From: Eric Paris Date: Tue, 9 Jun 2015 18:26:25 -0400 Subject: [PATCH 4/5] Make error handling more obvious Again, the code looks a little more like a middle-schooler's code. But that just makes it easier to understand and maintain. --- cobra_test.go | 2 +- command.go | 31 ++++++++++++++++--------------- 2 files changed, 17 insertions(+), 16 deletions(-) diff --git a/cobra_test.go b/cobra_test.go index f181a07..d7dd69f 100644 --- a/cobra_test.go +++ b/cobra_test.go @@ -768,7 +768,7 @@ func TestRootNoCommandHelp(t *testing.T) { func TestRootUnknownCommand(t *testing.T) { r := noRRSetupTest("bogus") - s := "Error: unknown command \"bogus\"\nRun 'cobra-test help' for usage.\n" + s := "Error: unknown command \"bogus\"\nRun 'cobra-test --help' for usage.\n" if r.Output != s { t.Errorf("Unexpected response.\nExpecting to be:\n %q\nGot:\n %q\n", s, r.Output) diff --git a/command.go b/command.go index 4bcec0c..0bc1693 100644 --- a/command.go +++ b/command.go @@ -273,7 +273,7 @@ Additional help topics: {{if .HasHelpSubCommands}}{{range .Commands}}{{if and (not .Runnable) (not .Deprecated)}} {{rpad .CommandPath .CommandPathPadding}} {{.Short}}{{end}}{{end}}{{end}}{{if .HasRunnableSiblings }}{{range .Parent.Commands}}{{if and (not .Runnable) (not .Deprecated)}}{{if not (eq .Name $cmd.Name) }} {{rpad .CommandPath .CommandPathPadding}} {{.Short}}{{end}}{{end}}{{end}}{{end}} {{end}}{{ if .HasSubCommands }} -Use "{{.Root.Name}} help [command]" for more information about a command. +Use "{{.CommandPath}} [command] --help" for more information about a command. {{end}}` } } @@ -452,19 +452,13 @@ func (c *Command) execute(a []string) (err error) { } err = c.ParseFlags(a) - if err == flag.ErrHelp { - c.Help() - return nil - } if err != nil { - c.Usage() return err } - // If help is called, regardless of other flags, we print that. - // Print help also if c.Run is nil. + // If help is called, regardless of other flags, return we want help + // Also say we need help if c.Run is nil. if c.helpFlagVal || !c.Runnable() { - c.Help() - return nil + return flag.ErrHelp } c.preRun() @@ -544,13 +538,20 @@ func (c *Command) Execute() (err error) { } cmd, flags, err := c.Find(args) - if err == nil { - err = cmd.execute(flags) - } - if err != nil { c.Println("Error:", err.Error()) - c.Printf("Run '%v help' for usage.\n", c.Root().Name()) + c.Printf("Run '%v --help' for usage.\n", c.CommandPath()) + return err + } + + err = cmd.execute(flags) + if err != nil { + if err == flag.ErrHelp { + cmd.Help() + return nil + } + c.Println(cmd.UsageString()) + c.Println("Error:", err.Error()) } return From 9a9d01c9ec7271631d3b0d1e379ad654ab6da8a7 Mon Sep 17 00:00:00 2001 From: Eric Paris Date: Mon, 29 Jun 2015 17:05:49 -0400 Subject: [PATCH 5/5] Better error message Calling `cobra-test echo times one two turkey` where `one` and `two` are valid arguments but `turkey` is not now results in. Error: invalid argument "turkey" for "cobra-test echo times" Run 'cobra-test echo times --help' for usage. --- cobra_test.go | 2 +- command.go | 6 +++++- 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/cobra_test.go b/cobra_test.go index d7dd69f..7cb4917 100644 --- a/cobra_test.go +++ b/cobra_test.go @@ -768,7 +768,7 @@ func TestRootNoCommandHelp(t *testing.T) { func TestRootUnknownCommand(t *testing.T) { r := noRRSetupTest("bogus") - s := "Error: unknown command \"bogus\"\nRun 'cobra-test --help' for usage.\n" + s := "Error: unknown command \"bogus\" for \"cobra-test\"\nRun 'cobra-test --help' for usage.\n" if r.Output != s { t.Errorf("Unexpected response.\nExpecting to be:\n %q\nGot:\n %q\n", s, r.Output) diff --git a/command.go b/command.go index 0bc1693..e82fd88 100644 --- a/command.go +++ b/command.go @@ -422,7 +422,7 @@ func (c *Command) Find(args []string) (*Command, []string, error) { } // root command with subcommands, do subcommand checking if commandFound == c && len(argsWOflags) > 0 { - return nil, a, fmt.Errorf("unknown command %q", argsWOflags[0]) + return commandFound, a, fmt.Errorf("unknown command %q for %q", argsWOflags[0], commandFound.CommandPath()) } return commandFound, a, nil @@ -539,6 +539,10 @@ func (c *Command) Execute() (err error) { cmd, flags, err := c.Find(args) if err != nil { + // If found parse to a subcommand and then failed, talk about the subcommand + if cmd != nil { + c = cmd + } c.Println("Error:", err.Error()) c.Printf("Run '%v --help' for usage.\n", c.CommandPath()) return err