From e01aafce269dca7d8a6cff6d619f708ef29958db Mon Sep 17 00:00:00 2001 From: Jawher Moussa Date: Wed, 16 Aug 2017 13:17:35 +0200 Subject: [PATCH] Only look for help and version flags in the first argument (#42) " mow.cli" used to search for -h, --help, -v, --version, ... shortcuts in all the arguments. This causes an issue with `--` (all what follows is an arg), e.g.: ``` app -- -h ``` Would still display the app help instead of treating `-h` as the argument it is. Also, this PR changes how the library handles encountering help and version flags. Instead of calling os.exit, the value of `errorHandling` is now used to decide. If it is `ContinueOnError`, than exit is not called and an nil error is returned from `app.Run`. The old behaviour is still available with `ExitOnError`. Fixes #41 --- cli.go | 4 +-- cli_test.go | 92 +++++++++++++++++++++++++++++++++++++++++++++++------ commands.go | 50 ++++++++++++++--------------- errors.go | 10 ++++++ 4 files changed, 120 insertions(+), 36 deletions(-) create mode 100644 errors.go diff --git a/cli.go b/cli.go index 7992645..6b9ea76 100644 --- a/cli.go +++ b/cli.go @@ -67,14 +67,14 @@ func (cli *Cli) parse(args []string, entry, inFlow, outFlow *step) error { // After that, we just call Cmd.parse() for the default behavior if cli.versionSetAndRequested(args) { cli.PrintVersion() - exiter(0) + cli.onError(errVersionRequested) return nil } return cli.Cmd.parse(args, entry, inFlow, outFlow) } func (cli *Cli) versionSetAndRequested(args []string) bool { - return cli.version != nil && cli.isArgSet(args, cli.version.option.names) + return cli.version != nil && cli.isFlagSet(args, cli.version.option.names) } /* diff --git a/cli_test.go b/cli_test.go index 498c4c6..1aa7d50 100644 --- a/cli_test.go +++ b/cli_test.go @@ -47,25 +47,36 @@ func TestImplicitSpec(t *testing.T) { require.True(t, called, "Exec wasn't called") } -func TestHelpShortcut(t *testing.T) { +func testHelpAndVersionWithOptionsEnd(flag string, t *testing.T) { + t.Logf("Testing help/version with --: flag=%q", flag) defer suppressOutput()() exitCalled := false - defer exitShouldBeCalledWith(t, 2, &exitCalled)() + defer exitShouldBeCalledWith(t, 0, &exitCalled)() app := App("x", "") - app.Spec = "Y" + app.Version("v version", "1.0") + app.Spec = "CMD" - app.String(StringArg{Name: "Y", Value: "", Desc: ""}) + cmd := app.String(StringArg{Name: "CMD", Value: "", Desc: ""}) actionCalled := false app.Action = func() { actionCalled = true + require.Equal(t, flag, *cmd) } - app.Run([]string{"x", "y", "-h", "z"}) - require.False(t, actionCalled, "action should not have been called") - require.True(t, exitCalled, "exit should have been called") + app.Run([]string{"x", "--", flag}) + + require.True(t, actionCalled, "action should have been called") + require.False(t, exitCalled, "exit should not have been called") + +} + +func TestHelpAndVersionWithOptionsEnd(t *testing.T) { + for _, flag := range []string{"-h", "--help", "-v", "--version"} { + testHelpAndVersionWithOptionsEnd(flag, t) + } } func TestHelpMessage(t *testing.T) { @@ -73,7 +84,7 @@ func TestHelpMessage(t *testing.T) { defer captureAndRestoreOutput(&out, &err)() exitCalled := false - defer exitShouldBeCalledWith(t, 2, &exitCalled)() + defer exitShouldBeCalledWith(t, 0, &exitCalled)() app := App("app", "App Desc") app.Spec = "[-o] ARG" @@ -104,7 +115,7 @@ func TestLongHelpMessage(t *testing.T) { defer captureAndRestoreOutput(&out, &err)() exitCalled := false - defer exitShouldBeCalledWith(t, 2, &exitCalled)() + defer exitShouldBeCalledWith(t, 0, &exitCalled)() app := App("app", "App Desc") app.LongDesc = "Longer App Desc" @@ -190,6 +201,34 @@ func TestContinueOnError(t *testing.T) { require.False(t, called, "Exec should NOT have been called") } +func TestContinueOnErrorWithHelpAndVersion(t *testing.T) { + defer exitShouldNotCalled(t)() + defer suppressOutput()() + + app := App("say", "") + app.Version("v", "1.0") + app.String(StringOpt{Name: "f", Value: "", Desc: ""}) + app.Spec = "-f" + app.ErrorHandling = flag.ContinueOnError + called := false + app.Action = func() { + called = true + } + + { + err := app.Run([]string{"say", "-h"}) + require.Nil(t, err) + require.False(t, called, "Exec should NOT have been called") + } + + { + err := app.Run([]string{"say", "-v"}) + require.Nil(t, err) + require.False(t, called, "Exec should NOT have been called") + } + +} + func TestExitOnError(t *testing.T) { defer suppressOutput()() @@ -197,13 +236,48 @@ func TestExitOnError(t *testing.T) { defer exitShouldBeCalledWith(t, 2, &exitCalled)() app := App("x", "") + app.ErrorHandling = flag.ExitOnError app.Spec = "Y" app.String(StringArg{Name: "Y", Value: "", Desc: ""}) + app.Run([]string{"x", "y", "z"}) require.True(t, exitCalled, "exit should have been called") } +func TestExitOnErrorWithHelp(t *testing.T) { + defer suppressOutput()() + + exitCalled := false + defer exitShouldBeCalledWith(t, 0, &exitCalled)() + + app := App("x", "") + app.Spec = "Y" + app.ErrorHandling = flag.ExitOnError + + app.String(StringArg{Name: "Y", Value: "", Desc: ""}) + + app.Run([]string{"x", "-h"}) + require.True(t, exitCalled, "exit should have been called") +} + +func TestExitOnErrorWithVersion(t *testing.T) { + defer suppressOutput()() + + exitCalled := false + defer exitShouldBeCalledWith(t, 0, &exitCalled)() + + app := App("x", "") + app.Version("v", "1.0") + app.Spec = "Y" + app.ErrorHandling = flag.ExitOnError + + app.String(StringArg{Name: "Y", Value: "", Desc: ""}) + + app.Run([]string{"x", "-v"}) + require.True(t, exitCalled, "exit should have been called") +} + func TestPanicOnError(t *testing.T) { defer suppressOutput()() diff --git a/commands.go b/commands.go index 28e831e..6195a9b 100644 --- a/commands.go +++ b/commands.go @@ -275,18 +275,20 @@ func (c *Cmd) doInit() error { } func (c *Cmd) onError(err error) { - if err != nil { - switch c.ErrorHandling { - case flag.ExitOnError: - exiter(2) - case flag.PanicOnError: - panic(err) - } - } else { + if err == errHelpRequested || err == errVersionRequested { if c.ErrorHandling == flag.ExitOnError { - exiter(2) + exiter(0) } + return + } + + switch c.ErrorHandling { + case flag.ExitOnError: + exiter(2) + case flag.PanicOnError: + panic(err) } + } /* @@ -401,7 +403,7 @@ func (c *Cmd) formatDescription(desc, envVar string) string { func (c *Cmd) parse(args []string, entry, inFlow, outFlow *step) error { if c.helpRequested(args) { c.PrintLongHelp() - c.onError(nil) + c.onError(errHelpRequested) return nil } @@ -471,26 +473,24 @@ func (c *Cmd) parse(args []string, entry, inFlow, outFlow *step) error { } -func (c *Cmd) isArgSet(args []string, searchArgs []string) bool { - for _, arg := range args { - for _, sub := range c.commands { - if sub.isAlias(arg) { - return false - } - } - for _, searchArg := range searchArgs { - if arg == searchArg { - return true - } +func (c *Cmd) helpRequested(args []string) bool { + return c.isFlagSet(args, []string{"-h", "--help"}) +} + +func (c *Cmd) isFlagSet(args []string, searchArgs []string) bool { + if len(args) == 0 { + return false + } + + arg := args[0] + for _, searchArg := range searchArgs { + if arg == searchArg { + return true } } return false } -func (c *Cmd) helpRequested(args []string) bool { - return c.isArgSet(args, []string{"-h", "--help"}) -} - func (c *Cmd) getOptsAndArgs(args []string) int { consumed := 0 diff --git a/errors.go b/errors.go new file mode 100644 index 0000000..b2e21fc --- /dev/null +++ b/errors.go @@ -0,0 +1,10 @@ +package cli + +import ( + "errors" +) + +var ( + errHelpRequested = errors.New("Help requested") + errVersionRequested = errors.New("Version requested") +)