From f585ec7cb8b0d9189ac9016fe83f911f3a360f2e Mon Sep 17 00:00:00 2001 From: Jesse Szwedko Date: Sat, 7 May 2016 16:36:54 -0700 Subject: [PATCH] Remove reordering of flags and arguments This was introduced by #36, but only worked in the specific case of all arguments being passed before all flags. If the user mixed them, they ended up with odd parsing behavior where the arguments were reordered (causing #103 and #355). Given the tradeoffs I think we should remove support for flag reordering. Fixes #103 Fixes #355 --- CHANGELOG.md | 11 +++++++++-- app_test.go | 29 +++-------------------------- command.go | 38 +++----------------------------------- command_test.go | 8 ++++---- 4 files changed, 19 insertions(+), 67 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index ea6c483..b0e1d48 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,8 +8,15 @@ ### Removed - the ability to specify `&StringSlice{...string}` or `&IntSlice{...int}`. -To migrate to the new API, you may choose to run [this helper -(python) script](./cli-migrate-slice-types). + To migrate to the new API, you may choose to run [this helper + (python) script](./cli-migrate-slice-types). +- The optimistic reordering of arguments and flags introduced by + https://github.com/codegangsta/cli/pull/36. This behavior only worked when + all arguments appeared before all flags, but caused [weird issues with boolean + flags](https://github.com/codegangsta/cli/issues/103) and [reordering of the + arguments](https://github.com/codegangsta/cli/issues/355) when the user + attempted to mix flags and arguments. Given the trade-offs we removed support + for this reordering. ## [Unreleased] - (1.x series) ### Added diff --git a/app_test.go b/app_test.go index ca4abce..a66cfa4 100644 --- a/app_test.go +++ b/app_test.go @@ -189,29 +189,6 @@ func TestApp_Command(t *testing.T) { } } -func TestApp_CommandWithArgBeforeFlags(t *testing.T) { - var parsedOption, firstArg string - - app := NewApp() - command := Command{ - Name: "cmd", - Flags: []Flag{ - StringFlag{Name: "option", Value: "", Usage: "some option"}, - }, - Action: func(c *Context) error { - parsedOption = c.String("option") - firstArg = c.Args().First() - return nil - }, - } - app.Commands = []Command{command} - - app.Run([]string{"", "cmd", "my-arg", "--option", "my-option"}) - - expect(t, parsedOption, "my-option") - expect(t, firstArg, "my-arg") -} - func TestApp_RunAsSubcommandParseFlags(t *testing.T) { var context *Context @@ -257,7 +234,7 @@ func TestApp_CommandWithFlagBeforeTerminator(t *testing.T) { } app.Commands = []Command{command} - app.Run([]string{"", "cmd", "my-arg", "--option", "my-option", "--", "--notARealFlag"}) + app.Run([]string{"", "cmd", "--option", "my-option", "my-arg", "--", "--notARealFlag"}) expect(t, parsedOption, "my-option") expect(t, args[0], "my-arg") @@ -342,7 +319,7 @@ func TestApp_ParseSliceFlags(t *testing.T) { } app.Commands = []Command{command} - app.Run([]string{"", "cmd", "my-arg", "-p", "22", "-p", "80", "-ip", "8.8.8.8", "-ip", "8.8.4.4"}) + app.Run([]string{"", "cmd", "-p", "22", "-p", "80", "-ip", "8.8.8.8", "-ip", "8.8.4.4", "my-arg"}) IntsEquals := func(a, b []int) bool { if len(a) != len(b) { @@ -398,7 +375,7 @@ func TestApp_ParseSliceFlagsWithMissingValue(t *testing.T) { } app.Commands = []Command{command} - app.Run([]string{"", "cmd", "my-arg", "-a", "2", "-str", "A"}) + app.Run([]string{"", "cmd", "-a", "2", "-str", "A", "my-arg"}) var expectedIntSlice = []int{2} var expectedStringSlice = []string{"A"} diff --git a/command.go b/command.go index 9ca7e51..7c94de3 100644 --- a/command.go +++ b/command.go @@ -86,42 +86,10 @@ func (c Command) Run(ctx *Context) (err error) { set := flagSet(c.Name, c.Flags) set.SetOutput(ioutil.Discard) - if !c.SkipFlagParsing { - firstFlagIndex := -1 - terminatorIndex := -1 - for index, arg := range ctx.Args() { - if arg == "--" { - terminatorIndex = index - break - } else if arg == "-" { - // Do nothing. A dash alone is not really a flag. - continue - } else if strings.HasPrefix(arg, "-") && firstFlagIndex == -1 { - firstFlagIndex = index - } - } - - if firstFlagIndex > -1 { - args := ctx.Args() - regularArgs := make([]string, len(args[1:firstFlagIndex])) - copy(regularArgs, args[1:firstFlagIndex]) - - var flagArgs []string - if terminatorIndex > -1 { - flagArgs = args[firstFlagIndex:terminatorIndex] - regularArgs = append(regularArgs, args[terminatorIndex:]...) - } else { - flagArgs = args[firstFlagIndex:] - } - - err = set.Parse(append(flagArgs, regularArgs...)) - } else { - err = set.Parse(ctx.Args().Tail()) - } + if c.SkipFlagParsing { + err = set.Parse(append([]string{"--"}, ctx.Args().Tail()...)) } else { - if c.SkipFlagParsing { - err = set.Parse(append([]string{"--"}, ctx.Args().Tail()...)) - } + err = set.Parse(ctx.Args().Tail()) } if err != nil { diff --git a/command_test.go b/command_test.go index 2687212..5e51843 100644 --- a/command_test.go +++ b/command_test.go @@ -15,10 +15,10 @@ func TestCommandFlagParsing(t *testing.T) { skipFlagParsing bool expectedErr error }{ - {[]string{"blah", "blah", "-break"}, false, errors.New("flag provided but not defined: -break")}, // Test normal "not ignoring flags" flow - {[]string{"blah", "blah"}, true, nil}, // Test SkipFlagParsing without any args that look like flags - {[]string{"blah", "-break"}, true, nil}, // Test SkipFlagParsing with random flag arg - {[]string{"blah", "-help"}, true, nil}, // Test SkipFlagParsing with "special" help flag arg + {[]string{"test-cmd", "-break", "blah", "blah"}, false, errors.New("flag provided but not defined: -break")}, // Test normal "not ignoring flags" flow + {[]string{"test-cmd", "blah", "blah"}, true, nil}, // Test SkipFlagParsing without any args that look like flags + {[]string{"test-cmd", "-break", "blah"}, true, nil}, // Test SkipFlagParsing with random flag arg + {[]string{"test-cmd", "-help", "blah"}, true, nil}, // Test SkipFlagParsing with "special" help flag arg } for _, c := range cases {