Merge pull request #398 from codegangsta/do-not-reorder-flags

Remove reordering of flags and arguments

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

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"}

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())
-		}
-	} else {
 	if c.SkipFlagParsing {
 		err = set.Parse(append([]string{"--"}, ctx.Args().Tail()...))
-		}
+	} else {
+		err = set.Parse(ctx.Args().Tail())
 	}
 
 	if err != nil {

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 {