From 4381738fb5799d9300598562adbb86c0944d8eb6 Mon Sep 17 00:00:00 2001 From: "[[ BOT ]] Lynn Cyrin" Date: Thu, 22 Aug 2019 20:43:52 -0700 Subject: [PATCH 01/26] wip regression test --- app_test.go | 37 +++++++++++++++++++++++++++++++++++++ 1 file changed, 37 insertions(+) diff --git a/app_test.go b/app_test.go index bccede4959..a0d17d1385 100644 --- a/app_test.go +++ b/app_test.go @@ -1187,6 +1187,43 @@ func TestRequiredFlagAppRunBehavior(t *testing.T) { } } +func TestAppRunPassThroughRegression(t *testing.T) { + tdata := []struct { + testCase string + appFlags []Flag + appRunInput []string + appCommands []Command + expectedAnError bool + }{ + { // docker run --rm ubuntu bash + appRunInput: []string{"myCLI", "myCommand", "--someFlag", "someInput", "docker", "run", "--rm", "ubuntu", "bash"}, + appCommands: []Command{{ + Name: "myCommand", + Flags: []Flag{StringFlag{Name: "someFlag"}}, + }}, + }, + } + for _, test := range tdata { + t.Run(test.testCase, func(t *testing.T) { + // setup + app := NewApp() + app.Flags = test.appFlags + app.Commands = test.appCommands + + // logic under test + err := app.Run(test.appRunInput) + + // assertions + if test.expectedAnError && err == nil { + t.Errorf("expected an error, but there was none") + } + if !test.expectedAnError && err != nil { + t.Errorf("did not expected an error, but there was one: %s", err) + } + }) + } +} + func TestAppHelpPrinter(t *testing.T) { oldPrinter := HelpPrinter defer func() { From e265f5087fd795a5314286e8e5ec19d91f58f4ae Mon Sep 17 00:00:00 2001 From: "[[ BOT ]] Lynn Cyrin" Date: Thu, 22 Aug 2019 21:22:37 -0700 Subject: [PATCH 02/26] cleanup tests --- app_test.go | 17 ++++++----------- 1 file changed, 6 insertions(+), 11 deletions(-) diff --git a/app_test.go b/app_test.go index a0d17d1385..9caa543b1e 100644 --- a/app_test.go +++ b/app_test.go @@ -1189,13 +1189,12 @@ func TestRequiredFlagAppRunBehavior(t *testing.T) { func TestAppRunPassThroughRegression(t *testing.T) { tdata := []struct { - testCase string - appFlags []Flag - appRunInput []string - appCommands []Command - expectedAnError bool + testCase string + appRunInput []string + appCommands []Command }{ - { // docker run --rm ubuntu bash + { + testCase: "test_case", appRunInput: []string{"myCLI", "myCommand", "--someFlag", "someInput", "docker", "run", "--rm", "ubuntu", "bash"}, appCommands: []Command{{ Name: "myCommand", @@ -1207,17 +1206,13 @@ func TestAppRunPassThroughRegression(t *testing.T) { t.Run(test.testCase, func(t *testing.T) { // setup app := NewApp() - app.Flags = test.appFlags app.Commands = test.appCommands // logic under test err := app.Run(test.appRunInput) // assertions - if test.expectedAnError && err == nil { - t.Errorf("expected an error, but there was none") - } - if !test.expectedAnError && err != nil { + if err != nil { t.Errorf("did not expected an error, but there was one: %s", err) } }) From 6e8917398c47b08f8069779668b8767f24abe559 Mon Sep 17 00:00:00 2001 From: "[[ BOT ]] Lynn Cyrin" Date: Thu, 22 Aug 2019 21:23:34 -0700 Subject: [PATCH 03/26] comment out tests --- app_test.go | 62 ++++++++++++++++++++++++++--------------------------- 1 file changed, 31 insertions(+), 31 deletions(-) diff --git a/app_test.go b/app_test.go index 9caa543b1e..84329eed81 100644 --- a/app_test.go +++ b/app_test.go @@ -1187,37 +1187,37 @@ func TestRequiredFlagAppRunBehavior(t *testing.T) { } } -func TestAppRunPassThroughRegression(t *testing.T) { - tdata := []struct { - testCase string - appRunInput []string - appCommands []Command - }{ - { - testCase: "test_case", - appRunInput: []string{"myCLI", "myCommand", "--someFlag", "someInput", "docker", "run", "--rm", "ubuntu", "bash"}, - appCommands: []Command{{ - Name: "myCommand", - Flags: []Flag{StringFlag{Name: "someFlag"}}, - }}, - }, - } - for _, test := range tdata { - t.Run(test.testCase, func(t *testing.T) { - // setup - app := NewApp() - app.Commands = test.appCommands - - // logic under test - err := app.Run(test.appRunInput) - - // assertions - if err != nil { - t.Errorf("did not expected an error, but there was one: %s", err) - } - }) - } -} +// func TestAppRunPassThroughRegression(t *testing.T) { +// tdata := []struct { +// testCase string +// appRunInput []string +// appCommands []Command +// }{ +// { +// testCase: "test_case", +// appRunInput: []string{"myCLI", "myCommand", "--someFlag", "someInput", "docker", "run", "--rm", "ubuntu", "bash"}, +// appCommands: []Command{{ +// Name: "myCommand", +// Flags: []Flag{StringFlag{Name: "someFlag"}}, +// }}, +// }, +// } +// for _, test := range tdata { +// t.Run(test.testCase, func(t *testing.T) { +// // setup +// app := NewApp() +// app.Commands = test.appCommands + +// // logic under test +// err := app.Run(test.appRunInput) + +// // assertions +// if err != nil { +// t.Errorf("did not expected an error, but there was one: %s", err) +// } +// }) +// } +// } func TestAppHelpPrinter(t *testing.T) { oldPrinter := HelpPrinter From 74cd3bc3fe03403d513c2084c48f7cbdd1d5bba6 Mon Sep 17 00:00:00 2001 From: "[[ BOT ]] Lynn Cyrin" Date: Thu, 22 Aug 2019 21:29:33 -0700 Subject: [PATCH 04/26] cleanup tests --- app_test.go | 47 ++++++++++++++++------------------------------- 1 file changed, 16 insertions(+), 31 deletions(-) diff --git a/app_test.go b/app_test.go index 84329eed81..37950f8dbf 100644 --- a/app_test.go +++ b/app_test.go @@ -1187,37 +1187,22 @@ func TestRequiredFlagAppRunBehavior(t *testing.T) { } } -// func TestAppRunPassThroughRegression(t *testing.T) { -// tdata := []struct { -// testCase string -// appRunInput []string -// appCommands []Command -// }{ -// { -// testCase: "test_case", -// appRunInput: []string{"myCLI", "myCommand", "--someFlag", "someInput", "docker", "run", "--rm", "ubuntu", "bash"}, -// appCommands: []Command{{ -// Name: "myCommand", -// Flags: []Flag{StringFlag{Name: "someFlag"}}, -// }}, -// }, -// } -// for _, test := range tdata { -// t.Run(test.testCase, func(t *testing.T) { -// // setup -// app := NewApp() -// app.Commands = test.appCommands - -// // logic under test -// err := app.Run(test.appRunInput) - -// // assertions -// if err != nil { -// t.Errorf("did not expected an error, but there was one: %s", err) -// } -// }) -// } -// } +func TestRegression(t *testing.T) { + // setup + app := NewApp() + app.Commands = []Command{{ + Name: "myCommand", + Flags: []Flag{StringFlag{Name: "someFlag"}}, + }} + + // logic under test + err := app.Run([]string{"myCLI", "myCommand", "--someFlag", "someInput", "docker", "run", "--rm", "ubuntu", "bash"}) + + // assertions + if err != nil { + t.Errorf("did not expected an error, but there was one: %s", err) + } +} func TestAppHelpPrinter(t *testing.T) { oldPrinter := HelpPrinter From 4fb1878febcd7f6e4b8c8b29b5ba9e7dfb77b370 Mon Sep 17 00:00:00 2001 From: "[[ BOT ]] Lynn Cyrin" Date: Thu, 22 Aug 2019 21:42:07 -0700 Subject: [PATCH 05/26] show test failures --- build.go | 68 +++++++++++++++++++++++++++++++++++++++++++++++--------- 1 file changed, 57 insertions(+), 11 deletions(-) diff --git a/build.go b/build.go index 2a38f8ba82..125d25f0a3 100644 --- a/build.go +++ b/build.go @@ -6,12 +6,13 @@ import ( "bufio" "bytes" "fmt" - "github.com/urfave/cli" "io/ioutil" "log" "os" "os/exec" "strings" + + "github.com/urfave/cli" ) var packages = []string{"cli", "altsrc"} @@ -52,7 +53,13 @@ func main() { } func VetActionFunc(_ *cli.Context) error { - return exec.Command("go", "vet").Run() + cmd := exec.Command("go", "vet") + + cmd.Stdin = os.Stdin + cmd.Stdout = os.Stdout + cmd.Stderr = os.Stderr + + return cmd.Run() } func TestActionFunc(c *cli.Context) error { @@ -67,10 +74,13 @@ func TestActionFunc(c *cli.Context) error { coverProfile := fmt.Sprintf("--coverprofile=%s.coverprofile", pkg) - err := exec.Command( - "go", "test", "-v", coverProfile, packageName, - ).Run() + cmd := exec.Command("go", "test", "-v", coverProfile, packageName) + cmd.Stdin = os.Stdin + cmd.Stdout = os.Stdout + cmd.Stderr = os.Stderr + + err := cmd.Run() if err != nil { return err } @@ -142,16 +152,34 @@ func GfmrunActionFunc(_ *cli.Context) error { return err } - return exec.Command("gfmrun", "-c", fmt.Sprint(counter), "-s", "README.md").Run() + cmd := exec.Command("gfmrun", "-c", fmt.Sprint(counter), "-s", "README.md") + + cmd.Stdin = os.Stdin + cmd.Stdout = os.Stdout + cmd.Stderr = os.Stderr + + return cmd.Run() } func TocActionFunc(_ *cli.Context) error { - err := exec.Command("node_modules/.bin/markdown-toc", "-i", "README.md").Run() + cmd := exec.Command("node_modules/.bin/markdown-toc", "-i", "README.md") + + cmd.Stdin = os.Stdin + cmd.Stdout = os.Stdout + cmd.Stderr = os.Stderr + + err := cmd.Run() if err != nil { return err } - err = exec.Command("git", "diff", "--exit-code").Run() + cmd = exec.Command("git", "diff", "--exit-code") + + cmd.Stdin = os.Stdin + cmd.Stdout = os.Stdout + cmd.Stderr = os.Stderr + + err = cmd.Run() if err != nil { return err } @@ -160,17 +188,35 @@ func TocActionFunc(_ *cli.Context) error { } func GenActionFunc(_ *cli.Context) error { - err := exec.Command("go", "generate", "flag-gen/main.go").Run() + cmd := exec.Command("go", "generate", "flag-gen/main.go") + + cmd.Stdin = os.Stdin + cmd.Stdout = os.Stdout + cmd.Stderr = os.Stderr + + err := cmd.Run() if err != nil { return err } - err = exec.Command("go", "generate", "cli.go").Run() + cmd = exec.Command("go", "generate", "cli.go") + + cmd.Stdin = os.Stdin + cmd.Stdout = os.Stdout + cmd.Stderr = os.Stderr + + err = cmd.Run() if err != nil { return err } - err = exec.Command("git", "diff", "--exit-code").Run() + cmd = exec.Command("git", "diff", "--exit-code") + + cmd.Stdin = os.Stdin + cmd.Stdout = os.Stdout + cmd.Stderr = os.Stderr + + err = cmd.Run() if err != nil { return err } From 15dc34ea12041906545f1466fd1af97c8f97ab69 Mon Sep 17 00:00:00 2001 From: Lynn Cyrin Date: Sun, 25 Aug 2019 08:54:06 -0700 Subject: [PATCH 06/26] more accurate regression reproduction --- app_test.go | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/app_test.go b/app_test.go index 37950f8dbf..2006bb89be 100644 --- a/app_test.go +++ b/app_test.go @@ -1191,12 +1191,17 @@ func TestRegression(t *testing.T) { // setup app := NewApp() app.Commands = []Command{{ - Name: "myCommand", - Flags: []Flag{StringFlag{Name: "someFlag"}}, + Name: "command", + Flags: []Flag{ + StringFlag{ + Name: "flagone", + }, + }, + Action: func(c *Context) error { return nil }, }} // logic under test - err := app.Run([]string{"myCLI", "myCommand", "--someFlag", "someInput", "docker", "run", "--rm", "ubuntu", "bash"}) + err := app.Run([]string{"cli", "command", "--flagone", "flagvalue", "docker", "image", "ls", "--no-trunc"}) // assertions if err != nil { From c542fb3bed0cbf052655db02a80ea97f03383421 Mon Sep 17 00:00:00 2001 From: Lynn Cyrin Date: Sun, 25 Aug 2019 09:04:51 -0700 Subject: [PATCH 07/26] temp remove --- app_test.go | 22 ---------------------- 1 file changed, 22 deletions(-) diff --git a/app_test.go b/app_test.go index 2006bb89be..bccede4959 100644 --- a/app_test.go +++ b/app_test.go @@ -1187,28 +1187,6 @@ func TestRequiredFlagAppRunBehavior(t *testing.T) { } } -func TestRegression(t *testing.T) { - // setup - app := NewApp() - app.Commands = []Command{{ - Name: "command", - Flags: []Flag{ - StringFlag{ - Name: "flagone", - }, - }, - Action: func(c *Context) error { return nil }, - }} - - // logic under test - err := app.Run([]string{"cli", "command", "--flagone", "flagvalue", "docker", "image", "ls", "--no-trunc"}) - - // assertions - if err != nil { - t.Errorf("did not expected an error, but there was one: %s", err) - } -} - func TestAppHelpPrinter(t *testing.T) { oldPrinter := HelpPrinter defer func() { From b0ed044b01bed3c0e940840aeb9d314ffa117330 Mon Sep 17 00:00:00 2001 From: Lynn Cyrin Date: Sun, 25 Aug 2019 09:16:41 -0700 Subject: [PATCH 08/26] add regression test to its own file --- app_regression_test.go | 29 +++++++++++++++++++++++++++++ 1 file changed, 29 insertions(+) create mode 100644 app_regression_test.go diff --git a/app_regression_test.go b/app_regression_test.go new file mode 100644 index 0000000000..91e65aa68d --- /dev/null +++ b/app_regression_test.go @@ -0,0 +1,29 @@ +package cli + +import ( + "testing" +) + +// TestRegression tests a regression that was merged between versions 1.20.0 and 1.21.0 +// The included app.Run line worked in 1.20.0, and then was broken in 1.21.0. +func TestRegression(t *testing.T) { + // setup + app := NewApp() + app.Commands = []Command{{ + Name: "command", + Flags: []Flag{ + StringFlag{ + Name: "flagone", + }, + }, + Action: func(c *Context) error { return nil }, + }} + + // logic under test + err := app.Run([]string{"cli", "command", "--flagone", "flagvalue", "docker", "image", "ls", "--no-trunc"}) + + // assertions + if err != nil { + t.Errorf("did not expected an error, but there was one: %s", err) + } +} From 192ce003d9a36d0002b487f8d725e520f1db51cc Mon Sep 17 00:00:00 2001 From: Lynn Cyrin Date: Sun, 25 Aug 2019 09:17:16 -0700 Subject: [PATCH 09/26] expand name --- app_regression_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app_regression_test.go b/app_regression_test.go index 91e65aa68d..1dff45830f 100644 --- a/app_regression_test.go +++ b/app_regression_test.go @@ -6,7 +6,7 @@ import ( // TestRegression tests a regression that was merged between versions 1.20.0 and 1.21.0 // The included app.Run line worked in 1.20.0, and then was broken in 1.21.0. -func TestRegression(t *testing.T) { +func TestVersionOneTwoOneRegression(t *testing.T) { // setup app := NewApp() app.Commands = []Command{{ From 2172d382b6adcde4289a1ffcad9797c2f880da35 Mon Sep 17 00:00:00 2001 From: Lynn Cyrin Date: Sun, 25 Aug 2019 10:18:00 -0700 Subject: [PATCH 10/26] update tests --- app_regression_test.go | 49 +++++++++++++++++++++++++++++------------- 1 file changed, 34 insertions(+), 15 deletions(-) diff --git a/app_regression_test.go b/app_regression_test.go index 1dff45830f..dde2a1a7ce 100644 --- a/app_regression_test.go +++ b/app_regression_test.go @@ -6,24 +6,43 @@ import ( // TestRegression tests a regression that was merged between versions 1.20.0 and 1.21.0 // The included app.Run line worked in 1.20.0, and then was broken in 1.21.0. +// Relevant PR: https://github.com/urfave/cli/pull/872 func TestVersionOneTwoOneRegression(t *testing.T) { - // setup - app := NewApp() - app.Commands = []Command{{ - Name: "command", - Flags: []Flag{ - StringFlag{ - Name: "flagone", - }, + testData := []struct { + testCase string + appRunInput []string + }{ + // assertion: empty input, when a required flag is present, errors + { + testCase: "with_dash_dash", + appRunInput: []string{"cli", "command", "--flagone", "flagvalue", "--", "docker", "image", "ls", "--no-trunc"}, }, - Action: func(c *Context) error { return nil }, - }} + { + testCase: "without_dash_dash", + appRunInput: []string{"cli", "command", "--flagone", "flagvalue", "docker", "image", "ls", "--no-trunc"}, + }, + } + for _, test := range testData { + t.Run(test.testCase, func(t *testing.T) { + // setup + app := NewApp() + app.Commands = []Command{{ + Name: "command", + Flags: []Flag{ + StringFlag{ + Name: "flagone", + }, + }, + Action: func(c *Context) error { return nil }, + }} - // logic under test - err := app.Run([]string{"cli", "command", "--flagone", "flagvalue", "docker", "image", "ls", "--no-trunc"}) + // logic under test + err := app.Run(test.appRunInput) - // assertions - if err != nil { - t.Errorf("did not expected an error, but there was one: %s", err) + // assertions + if err != nil { + t.Errorf("did not expected an error, but there was one: %s", err) + } + }) } } From 810b9714d3eb5b8cd41a62527c39fb2730c6d151 Mon Sep 17 00:00:00 2001 From: Lynn Cyrin Date: Sun, 25 Aug 2019 10:34:15 -0700 Subject: [PATCH 11/26] cleanup --- app_regression_test.go | 1 - 1 file changed, 1 deletion(-) diff --git a/app_regression_test.go b/app_regression_test.go index dde2a1a7ce..2299df5109 100644 --- a/app_regression_test.go +++ b/app_regression_test.go @@ -12,7 +12,6 @@ func TestVersionOneTwoOneRegression(t *testing.T) { testCase string appRunInput []string }{ - // assertion: empty input, when a required flag is present, errors { testCase: "with_dash_dash", appRunInput: []string{"cli", "command", "--flagone", "flagvalue", "--", "docker", "image", "ls", "--no-trunc"}, From 0f9d8a9abdbb755069f45e89d90b3e06499302c2 Mon Sep 17 00:00:00 2001 From: Lynn Cyrin Date: Sun, 25 Aug 2019 11:14:20 -0700 Subject: [PATCH 12/26] expand test cases --- app_regression_test.go | 18 +++++++++++++++--- 1 file changed, 15 insertions(+), 3 deletions(-) diff --git a/app_regression_test.go b/app_regression_test.go index 2299df5109..3c8681b993 100644 --- a/app_regression_test.go +++ b/app_regression_test.go @@ -9,24 +9,36 @@ import ( // Relevant PR: https://github.com/urfave/cli/pull/872 func TestVersionOneTwoOneRegression(t *testing.T) { testData := []struct { - testCase string - appRunInput []string + testCase string + appRunInput []string + skipArgReorder bool }{ { testCase: "with_dash_dash", appRunInput: []string{"cli", "command", "--flagone", "flagvalue", "--", "docker", "image", "ls", "--no-trunc"}, }, + { + testCase: "with_dash_dash_and_skip_reorder", + appRunInput: []string{"cli", "command", "--flagone", "flagvalue", "--", "docker", "image", "ls", "--no-trunc"}, + skipArgReorder: true, + }, { testCase: "without_dash_dash", appRunInput: []string{"cli", "command", "--flagone", "flagvalue", "docker", "image", "ls", "--no-trunc"}, }, + { + testCase: "without_dash_dash_and_skip_reorder", + appRunInput: []string{"cli", "command", "--flagone", "flagvalue", "docker", "image", "ls", "--no-trunc"}, + skipArgReorder: true, + }, } for _, test := range testData { t.Run(test.testCase, func(t *testing.T) { // setup app := NewApp() app.Commands = []Command{{ - Name: "command", + Name: "command", + SkipArgReorder: test.skipArgReorder, Flags: []Flag{ StringFlag{ Name: "flagone", From 406a1c31b08754a4553634c1fbb0caaed22865b1 Mon Sep 17 00:00:00 2001 From: Lynn Cyrin Date: Sun, 25 Aug 2019 14:12:13 -0700 Subject: [PATCH 13/26] update vars and args --- command.go | 20 +++++++++++--------- 1 file changed, 11 insertions(+), 9 deletions(-) diff --git a/command.go b/command.go index 44a90de6b7..37387d1035 100644 --- a/command.go +++ b/command.go @@ -190,7 +190,7 @@ func (c *Command) parseFlags(args Args) (*flag.FlagSet, error) { } if !c.SkipArgReorder { - args = reorderArgs(args) + args = reorderArgs(c.Flags, args) } set, err := parseIter(c, args) @@ -214,34 +214,36 @@ func (c *Command) useShortOptionHandling() bool { return c.UseShortOptionHandling } -// reorderArgs moves all flags before arguments as this is what flag expects -func reorderArgs(args []string) []string { - var nonflags, flags []string +// reorderArgs moves all flags (reorderedFlags) before arguments (remainingArgs) +// as this is what flag expects. +func reorderArgs(commandFlags []Flag, args []string) []string { + var remainingArgs []string + var reorderedFlags []string readFlagValue := false for i, arg := range args { if arg == "--" { - nonflags = append(nonflags, args[i:]...) + remainingArgs = append(remainingArgs, args[i:]...) break } if readFlagValue && !strings.HasPrefix(arg, "-") && !strings.HasPrefix(arg, "--") { readFlagValue = false - flags = append(flags, arg) + reorderedFlags = append(reorderedFlags, arg) continue } readFlagValue = false if arg != "-" && strings.HasPrefix(arg, "-") { - flags = append(flags, arg) + reorderedFlags = append(reorderedFlags, arg) readFlagValue = !strings.Contains(arg, "=") } else { - nonflags = append(nonflags, arg) + remainingArgs = append(remainingArgs, arg) } } - return append(flags, nonflags...) + return append(reorderedFlags, remainingArgs...) } // Names returns the names including short names and aliases. From 2071bcfb8328cb13e5575a586f06a33563b04054 Mon Sep 17 00:00:00 2001 From: Lynn Cyrin Date: Mon, 26 Aug 2019 00:18:36 -0700 Subject: [PATCH 14/26] checkpoint --- command.go | 21 +++++++++++++-------- 1 file changed, 13 insertions(+), 8 deletions(-) diff --git a/command.go b/command.go index 37387d1035..d33435e6dd 100644 --- a/command.go +++ b/command.go @@ -214,14 +214,18 @@ func (c *Command) useShortOptionHandling() bool { return c.UseShortOptionHandling } -// reorderArgs moves all flags (reorderedFlags) before arguments (remainingArgs) -// as this is what flag expects. +// reorderArgs moves all flags (via reorderedArgs) before the rest of +// the arguments (remainingArgs) as this is what flag expects. func reorderArgs(commandFlags []Flag, args []string) []string { var remainingArgs []string - var reorderedFlags []string + var reorderedArgs []string readFlagValue := false for i, arg := range args { + + // dont reorder any args after a -- + // read about -- here: + // https://unix.stackexchange.com/questions/11376/what-does-double-dash-mean-also-known-as-bare-double-dash if arg == "--" { remainingArgs = append(remainingArgs, args[i:]...) break @@ -229,21 +233,22 @@ func reorderArgs(commandFlags []Flag, args []string) []string { if readFlagValue && !strings.HasPrefix(arg, "-") && !strings.HasPrefix(arg, "--") { readFlagValue = false - reorderedFlags = append(reorderedFlags, arg) + reorderedArgs = append(reorderedArgs, arg) continue } readFlagValue = false if arg != "-" && strings.HasPrefix(arg, "-") { - reorderedFlags = append(reorderedFlags, arg) + reorderedArgs = append(reorderedArgs, arg) readFlagValue = !strings.Contains(arg, "=") - } else { - remainingArgs = append(remainingArgs, arg) + continue } + + remainingArgs = append(remainingArgs, arg) } - return append(reorderedFlags, remainingArgs...) + return append(reorderedArgs, remainingArgs...) } // Names returns the names including short names and aliases. From 7d0751fa1309a01b405cde327cc43275d13016a2 Mon Sep 17 00:00:00 2001 From: Lynn Cyrin Date: Wed, 11 Sep 2019 19:25:51 -0700 Subject: [PATCH 15/26] add argIsFlag check --- command.go | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-) diff --git a/command.go b/command.go index d33435e6dd..6471f1005b 100644 --- a/command.go +++ b/command.go @@ -238,7 +238,7 @@ func reorderArgs(commandFlags []Flag, args []string) []string { } readFlagValue = false - if arg != "-" && strings.HasPrefix(arg, "-") { + if arg != "-" && strings.HasPrefix(arg, "-") && argIsFlag(commandFlags, arg) { reorderedArgs = append(reorderedArgs, arg) readFlagValue = !strings.Contains(arg, "=") @@ -251,6 +251,18 @@ func reorderArgs(commandFlags []Flag, args []string) []string { return append(reorderedArgs, remainingArgs...) } +func argIsFlag(commandFlags []Flag, arg string) bool { + strippedArg := strings.ReplaceAll(arg, "-", "") + for _, flag := range commandFlags { + for _, key := range strings.Split(flag.GetName(), ",") { + if key == strippedArg { + return true + } + } + } + return false +} + // Names returns the names including short names and aliases. func (c Command) Names() []string { names := []string{c.Name} From 49dbeed6877737d97ec6e121763a41c617d7b272 Mon Sep 17 00:00:00 2001 From: Lynn Cyrin Date: Wed, 11 Sep 2019 20:24:42 -0700 Subject: [PATCH 16/26] handle `=` input --- command.go | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/command.go b/command.go index 6471f1005b..d52080e3cb 100644 --- a/command.go +++ b/command.go @@ -252,10 +252,11 @@ func reorderArgs(commandFlags []Flag, args []string) []string { } func argIsFlag(commandFlags []Flag, arg string) bool { - strippedArg := strings.ReplaceAll(arg, "-", "") + arg = strings.ReplaceAll(arg, "-", "") + arg = strings.Split(arg, "=")[0] for _, flag := range commandFlags { for _, key := range strings.Split(flag.GetName(), ",") { - if key == strippedArg { + if key == arg { return true } } From 10682fbde6f00b1008d6748d7ffb090bce421da7 Mon Sep 17 00:00:00 2001 From: Lynn Cyrin Date: Wed, 11 Sep 2019 20:28:48 -0700 Subject: [PATCH 17/26] docs --- command.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/command.go b/command.go index d52080e3cb..af91dbcb7d 100644 --- a/command.go +++ b/command.go @@ -252,8 +252,11 @@ func reorderArgs(commandFlags []Flag, args []string) []string { } func argIsFlag(commandFlags []Flag, arg string) bool { + // this line turns `--flag` into `flag` arg = strings.ReplaceAll(arg, "-", "") + // this line turns `flag=value` into `flag` arg = strings.Split(arg, "=")[0] + // look through all the flags, to see if the `arg` is one of our flags for _, flag := range commandFlags { for _, key := range strings.Split(flag.GetName(), ",") { if key == arg { From 09cdbbfe281fd79e92616a28f1fdf701e9007c22 Mon Sep 17 00:00:00 2001 From: Lynn Cyrin Date: Wed, 11 Sep 2019 20:29:31 -0700 Subject: [PATCH 18/26] more docs --- command.go | 1 + 1 file changed, 1 insertion(+) diff --git a/command.go b/command.go index af91dbcb7d..6e09a324dd 100644 --- a/command.go +++ b/command.go @@ -264,6 +264,7 @@ func argIsFlag(commandFlags []Flag, arg string) bool { } } } + // return false if this arg was not one of our flags return false } From 223e21796c35c3f4a456db83c0d450ef5fe4db01 Mon Sep 17 00:00:00 2001 From: Lynn Cyrin Date: Wed, 11 Sep 2019 21:08:52 -0700 Subject: [PATCH 19/26] swap a test case --- command_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/command_test.go b/command_test.go index 56698fe1bb..47792e1fe6 100644 --- a/command_test.go +++ b/command_test.go @@ -18,7 +18,7 @@ func TestCommandFlagParsing(t *testing.T) { UseShortOptionHandling bool }{ // Test normal "not ignoring flags" flow - {[]string{"test-cmd", "blah", "blah", "-break"}, false, false, errors.New("flag provided but not defined: -break"), false}, + {[]string{"test-cmd", "blah", "blah", "-break"}, false, false, nil, false}, // Test no arg reorder {[]string{"test-cmd", "blah", "blah", "-break"}, false, true, nil, false}, From 6da413ad825dea1f74563bdfee1f8e5c23c1c650 Mon Sep 17 00:00:00 2001 From: Lynn Cyrin Date: Wed, 11 Sep 2019 22:45:51 -0700 Subject: [PATCH 20/26] fix go version support? --- command.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/command.go b/command.go index 6e09a324dd..0a6b6ed0fe 100644 --- a/command.go +++ b/command.go @@ -253,7 +253,7 @@ func reorderArgs(commandFlags []Flag, args []string) []string { func argIsFlag(commandFlags []Flag, arg string) bool { // this line turns `--flag` into `flag` - arg = strings.ReplaceAll(arg, "-", "") + arg = strings.Replace(arg, "-", "", -1) // this line turns `flag=value` into `flag` arg = strings.Split(arg, "=")[0] // look through all the flags, to see if the `arg` is one of our flags From bfdd794eb3d0f8d99af8939a3a8c24547ef262ca Mon Sep 17 00:00:00 2001 From: Lynn Cyrin Date: Wed, 11 Sep 2019 23:05:26 -0700 Subject: [PATCH 21/26] docs --- command.go | 26 ++++++++++++++------------ 1 file changed, 14 insertions(+), 12 deletions(-) diff --git a/command.go b/command.go index 0a6b6ed0fe..3993e0ff15 100644 --- a/command.go +++ b/command.go @@ -220,7 +220,7 @@ func reorderArgs(commandFlags []Flag, args []string) []string { var remainingArgs []string var reorderedArgs []string - readFlagValue := false + nextIndexMayContainValue := false for i, arg := range args { // dont reorder any args after a -- @@ -229,28 +229,30 @@ func reorderArgs(commandFlags []Flag, args []string) []string { if arg == "--" { remainingArgs = append(remainingArgs, args[i:]...) break - } - if readFlagValue && !strings.HasPrefix(arg, "-") && !strings.HasPrefix(arg, "--") { - readFlagValue = false + // checks if this arg is a value that should be re-ordered next to its associated flag + } else if nextIndexMayContainValue && !strings.HasPrefix(arg, "-") { + nextIndexMayContainValue = false reorderedArgs = append(reorderedArgs, arg) - continue - } - readFlagValue = false - if arg != "-" && strings.HasPrefix(arg, "-") && argIsFlag(commandFlags, arg) { + // checks if this is an arg that should be re-ordered + } else if arg != "-" && strings.HasPrefix(arg, "-") && argIsFlag(commandFlags, arg) { + + // we have determined that this is a flag that we should re-order reorderedArgs = append(reorderedArgs, arg) + // if this arg does not contain a "=", then the next index may contain the value for this flag + nextIndexMayContainValue = !strings.Contains(arg, "=") - readFlagValue = !strings.Contains(arg, "=") - continue + // simply append any remaining args + } else { + remainingArgs = append(remainingArgs, arg) } - - remainingArgs = append(remainingArgs, arg) } return append(reorderedArgs, remainingArgs...) } +// argIsFlag checks if an arg is one of our command flags func argIsFlag(commandFlags []Flag, arg string) bool { // this line turns `--flag` into `flag` arg = strings.Replace(arg, "-", "", -1) From 3f6f97754aa8af1b6bd7b9324bf1e4c72023a269 Mon Sep 17 00:00:00 2001 From: "Lynn Cyrin (they/them)" Date: Fri, 13 Sep 2019 07:27:16 -0700 Subject: [PATCH 22/26] Update command.go Co-Authored-By: Sascha Grunert --- command.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/command.go b/command.go index 3993e0ff15..037abe97a1 100644 --- a/command.go +++ b/command.go @@ -217,7 +217,7 @@ func (c *Command) useShortOptionHandling() bool { // reorderArgs moves all flags (via reorderedArgs) before the rest of // the arguments (remainingArgs) as this is what flag expects. func reorderArgs(commandFlags []Flag, args []string) []string { - var remainingArgs []string + var remainingArgs, reorderedArgs []string var reorderedArgs []string nextIndexMayContainValue := false From 72c249389c590a6817f21cf21c68bc0a076823bf Mon Sep 17 00:00:00 2001 From: "Lynn Cyrin (they/them)" Date: Fri, 13 Sep 2019 07:35:16 -0700 Subject: [PATCH 23/26] Update command.go --- command.go | 1 - 1 file changed, 1 deletion(-) diff --git a/command.go b/command.go index 037abe97a1..9724a3bbfc 100644 --- a/command.go +++ b/command.go @@ -218,7 +218,6 @@ func (c *Command) useShortOptionHandling() bool { // the arguments (remainingArgs) as this is what flag expects. func reorderArgs(commandFlags []Flag, args []string) []string { var remainingArgs, reorderedArgs []string - var reorderedArgs []string nextIndexMayContainValue := false for i, arg := range args { From 51259808fff1f7384b556f6e127a090fe0362305 Mon Sep 17 00:00:00 2001 From: Lynn Cyrin Date: Tue, 1 Oct 2019 20:13:04 -0700 Subject: [PATCH 24/26] better leading dash handling --- command.go | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-) diff --git a/command.go b/command.go index dcaf2b4822..70502e87c9 100644 --- a/command.go +++ b/command.go @@ -240,8 +240,7 @@ func reorderArgs(commandFlags []Flag, args []string) []string { reorderedArgs = append(reorderedArgs, arg) // checks if this is an arg that should be re-ordered - } else if arg != "-" && strings.HasPrefix(arg, "-") && argIsFlag(commandFlags, arg) { - + } else if argIsFlag(commandFlags, arg) { // we have determined that this is a flag that we should re-order reorderedArgs = append(reorderedArgs, arg) // if this arg does not contain a "=", then the next index may contain the value for this flag @@ -258,8 +257,18 @@ func reorderArgs(commandFlags []Flag, args []string) []string { // argIsFlag checks if an arg is one of our command flags func argIsFlag(commandFlags []Flag, arg string) bool { + // checks if this is just a `-`, and so definitely not a flag + if arg == "-" { + return false + } // this line turns `--flag` into `flag` - arg = strings.Replace(arg, "-", "", -1) + if strings.HasPrefix(arg, "--") { + arg = strings.Replace(arg, "-", "", 2) + } + // this line turns `-flag` into `flag` + if strings.HasPrefix(arg, "-") { + arg = strings.Replace(arg, "-", "", 1) + } // this line turns `flag=value` into `flag` arg = strings.Split(arg, "=")[0] // look through all the flags, to see if the `arg` is one of our flags From be0cc4b8715d2a57f03b395ae928db61048bb679 Mon Sep 17 00:00:00 2001 From: Lynn Cyrin Date: Tue, 1 Oct 2019 20:18:53 -0700 Subject: [PATCH 25/26] add a check --- command.go | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/command.go b/command.go index 70502e87c9..d41a1bf132 100644 --- a/command.go +++ b/command.go @@ -261,6 +261,10 @@ func argIsFlag(commandFlags []Flag, arg string) bool { if arg == "-" { return false } + // flags always start with a - + if !strings.HasPrefix(arg, "-") { + return false + } // this line turns `--flag` into `flag` if strings.HasPrefix(arg, "--") { arg = strings.Replace(arg, "-", "", 2) From 64d3555482218336121cd318c27454041cc479a1 Mon Sep 17 00:00:00 2001 From: Lynn Cyrin Date: Tue, 1 Oct 2019 20:21:12 -0700 Subject: [PATCH 26/26] trim whitespace --- command.go | 1 + 1 file changed, 1 insertion(+) diff --git a/command.go b/command.go index d41a1bf132..e7cb97a6af 100644 --- a/command.go +++ b/command.go @@ -278,6 +278,7 @@ func argIsFlag(commandFlags []Flag, arg string) bool { // look through all the flags, to see if the `arg` is one of our flags for _, flag := range commandFlags { for _, key := range strings.Split(flag.GetName(), ",") { + key := strings.TrimSpace(key) if key == arg { return true }