From df562bf1a8626f2d16f91fcbf7230a5bdca3d592 Mon Sep 17 00:00:00 2001 From: Jesse Szwedko Date: Sun, 3 Dec 2017 13:38:50 -0800 Subject: [PATCH 1/2] Refactor flag handling logic Refactor logic introduced by #686 --- command.go | 134 +++++++++++++++++++++-------------------------------- 1 file changed, 52 insertions(+), 82 deletions(-) diff --git a/command.go b/command.go index 7d0357b..66a58b5 100644 --- a/command.go +++ b/command.go @@ -1,6 +1,7 @@ package cli import ( + "flag" "fmt" "io/ioutil" "sort" @@ -110,43 +111,7 @@ func (c Command) Run(ctx *Context) (err error) { ) } - set, err := flagSet(c.Name, c.Flags) - if err != nil { - return err - } - set.SetOutput(ioutil.Discard) - firstFlagIndex, terminatorIndex := getIndexes(ctx) - flagArgs, regularArgs := getAllArgs(ctx.Args(), firstFlagIndex, terminatorIndex) - if c.UseShortOptionHandling { - flagArgs = translateShortOptions(flagArgs) - } - if c.SkipFlagParsing { - err = set.Parse(append([]string{"--"}, ctx.Args().Tail()...)) - } else if !c.SkipArgReorder { - if firstFlagIndex > -1 { - err = set.Parse(append(flagArgs, regularArgs...)) - } else { - err = set.Parse(ctx.Args().Tail()) - } - } else if c.UseShortOptionHandling { - if terminatorIndex == -1 && firstFlagIndex > -1 { - // Handle shortname AND no options - err = set.Parse(append(regularArgs, flagArgs...)) - } else { - // Handle shortname and options - err = set.Parse(flagArgs) - } - } else { - err = set.Parse(append(regularArgs, flagArgs...)) - } - - nerr := normalizeFlags(c.Flags, set) - if nerr != nil { - fmt.Fprintln(ctx.App.Writer, nerr) - fmt.Fprintln(ctx.App.Writer) - ShowCommandHelp(ctx, c.Name) - return nerr - } + set, err := c.parseFlags(ctx.Args().Tail()) context := NewContext(ctx.App, set, ctx) context.Command = c @@ -205,60 +170,65 @@ func (c Command) Run(ctx *Context) (err error) { return err } -func getIndexes(ctx *Context) (int, int) { - 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 - } +func (c *Command) parseFlags(args Args) (*flag.FlagSet, error) { + set, err := flagSet(c.Name, c.Flags) + if err != nil { + return nil, err } - if len(ctx.Args()) > 0 && !strings.HasPrefix(ctx.Args()[0], "-") && firstFlagIndex == -1 { - return -1, -1 + set.SetOutput(ioutil.Discard) + + if c.SkipFlagParsing { + return set, set.Parse(append([]string{c.Name, "--"}, args...)) } - return firstFlagIndex, terminatorIndex + if c.UseShortOptionHandling { + args = translateShortOptions(args) + } -} + if !c.SkipArgReorder { + args = reorderArgs(args) + } -// copyStringslice takes a string slice and copies it -func copyStringSlice(slice []string, start, end int) []string { - newSlice := make([]string, end-start) - copy(newSlice, slice[start:end]) - return newSlice -} + err = set.Parse(args) + if err != nil { + return nil, err + } -// getAllArgs extracts and returns two string slices representing -// regularArgs and flagArgs -func getAllArgs(args []string, firstFlagIndex, terminatorIndex int) ([]string, []string) { - var regularArgs []string - // if there are no options, the we set the index to 1 manually - if firstFlagIndex == -1 { - firstFlagIndex = 1 - regularArgs = copyStringSlice(args, 0, len(args)) - } else { - regularArgs = copyStringSlice(args, 1, firstFlagIndex) + err = normalizeFlags(c.Flags, set) + if err != nil { + return nil, err } - var flagArgs []string - // a flag terminatorIndex was found in the input. we need to collect - // flagArgs based on it. - if terminatorIndex > -1 { - flagArgs = copyStringSlice(args, firstFlagIndex, terminatorIndex) - additionalRegularArgs := copyStringSlice(args, terminatorIndex, len(args)) - regularArgs = append(regularArgs, additionalRegularArgs...) - for _, i := range additionalRegularArgs { - regularArgs = append(regularArgs, i) + + return set, nil +} + +// reorderArgs moves all flags before arguments as this is what flag expects +func reorderArgs(args []string) []string { + var nonflags, flags []string + + readFlagValue := false + for i, arg := range args { + if arg == "--" { + nonflags = append(nonflags, args[i:]...) + break + } + + if readFlagValue { + readFlagValue = false + flags = append(flags, arg) + continue + } + + if arg != "-" && strings.HasPrefix(arg, "-") { + flags = append(flags, arg) + + readFlagValue = !strings.Contains(arg, "=") + } else { + nonflags = append(nonflags, arg) } - } else { - flagArgs = args[firstFlagIndex:] } - return flagArgs, regularArgs + + return append(flags, nonflags...) } func translateShortOptions(flagArgs Args) []string { From 0671b166dcacb3dc1215ba65bf986dab194581dc Mon Sep 17 00:00:00 2001 From: Jesse Szwedko Date: Mon, 4 Dec 2017 09:23:40 -0800 Subject: [PATCH 2/2] Add tests for flag reordering --- command_test.go | 38 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 38 insertions(+) diff --git a/command_test.go b/command_test.go index e69750a..c235e4a 100644 --- a/command_test.go +++ b/command_test.go @@ -243,3 +243,41 @@ func TestCommand_Run_SubcommandsCanUseErrWriter(t *testing.T) { t.Fatal(err) } } + +func TestCommandFlagReordering(t *testing.T) { + cases := []struct { + testArgs []string + expectedValue string + expectedArgs []string + expectedErr error + }{ + {[]string{"some-exec", "some-command", "some-arg", "--flag", "foo"}, "foo", []string{"some-arg"}, nil}, + {[]string{"some-exec", "some-command", "some-arg", "--flag=foo"}, "foo", []string{"some-arg"}, nil}, + {[]string{"some-exec", "some-command", "--flag=foo", "some-arg"}, "foo", []string{"some-arg"}, nil}, + } + + for _, c := range cases { + value := "" + args := []string{} + app := &App{ + Commands: []Command{ + { + Name: "some-command", + Flags: []Flag{ + StringFlag{Name: "flag"}, + }, + Action: func(c *Context) { + fmt.Printf("%+v\n", c.String("flag")) + value = c.String("flag") + args = c.Args() + }, + }, + }, + } + + err := app.Run(c.testArgs) + expect(t, err, c.expectedErr) + expect(t, value, c.expectedValue) + expect(t, args, c.expectedArgs) + } +}