From c5b9e989df31c5d19573e50d6188550ad51a971e Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Mon, 1 Sep 2025 21:10:46 +0200 Subject: [PATCH 1/3] remove uses of errors.Is, which requires go1.13 Commit 1bf832c6fec8a35a8c1d61e5fb14f5ce404197ef introduced the use of `errors.Is`, which was added in [go1.13], but the `go.mod` was not updated to reflect this, causing compile failures on go1.12: docker run -it --rm -v ./:/pflag -w /pflag golang:1.12 sh -c 'go test -v ./...' # github.com/spf13/pflag [github.com/spf13/pflag.test] ./flag.go:1190:7: undefined: errors.Is ./flag.go:1219:7: undefined: errors.Is ./bool_func_test.go:86:28: cannot use stdFSet (type *flag.FlagSet) as type BoolFuncFlagSet in argument to runCase: *flag.FlagSet does not implement BoolFuncFlagSet (missing BoolFunc method) ... As the error that is tested will not be wrapped, we can omit the `errors.Is`, and instead continue doing a straight comparison. [go1.13]: https://pkg.go.dev/errors@go1.13#Is Signed-off-by: Sebastiaan van Stijn --- flag.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/flag.go b/flag.go index eeed1e92..b9f08399 100644 --- a/flag.go +++ b/flag.go @@ -1185,7 +1185,7 @@ func (f *FlagSet) Parse(arguments []string) error { case ContinueOnError: return err case ExitOnError: - if errors.Is(err, ErrHelp) { + if err == ErrHelp { os.Exit(0) } fmt.Fprintln(f.Output(), err) @@ -1214,7 +1214,7 @@ func (f *FlagSet) ParseAll(arguments []string, fn func(flag *Flag, value string) case ContinueOnError: return err case ExitOnError: - if errors.Is(err, ErrHelp) { + if err == ErrHelp { os.Exit(0) } fmt.Fprintln(f.Output(), err) From 18a9d17d0ee8bd64d5c2071fc031be86fa2cd328 Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Mon, 1 Sep 2025 12:48:45 +0200 Subject: [PATCH 2/3] move Func, BoolFunc, tests as they require go1.21 Commit 69bc3bd5b37fa90e994be9acecf7430269591713 added support for Func() and BoolFunc() to match stdlib. However, the Func method was added in [go1.16.0], and BoolFunc in [go1.21.0], so running the tests on older versions of Go would fail; docker run -it --rm -v ./:/pflag -w /pflag golang:1.21 sh -c 'go test -v ./...' # github.com/spf13/pflag [github.com/spf13/pflag.test] ./bool_func_test.go:86:28: cannot use stdFSet (type *flag.FlagSet) as type BoolFuncFlagSet in argument to runCase: *flag.FlagSet does not implement BoolFuncFlagSet (missing BoolFunc method) ./bool_func_test.go:113:21: undefined: io.Discard ./bool_func_test.go:116:28: cannot use stdFSet (type *flag.FlagSet) as type BoolFuncFlagSet in argument to runCase: *flag.FlagSet does not implement BoolFuncFlagSet (missing BoolFunc method) ./bool_func_test.go:139:7: undefined: errors.Is ./func_test.go:92:28: cannot use stdFSet (type *flag.FlagSet) as type FuncFlagSet in argument to runCase: *flag.FlagSet does not implement FuncFlagSet (missing Func method) ./func_test.go:119:21: undefined: io.Discard ./func_test.go:122:28: cannot use stdFSet (type *flag.FlagSet) as type FuncFlagSet in argument to runCase: *flag.FlagSet does not implement FuncFlagSet (missing Func method) ./func_test.go:145:7: undefined: errors.Is ./func_test.go:145:7: too many errors FAIL github.com/spf13/pflag [build failed] This patch moves the tests to a separate file that is not built for older versions of Go. [go1.16.0]: https://pkg.go.dev/flag@go1.16.0#Func [go1.21.0]: https://pkg.go.dev/flag@go1.21.0#BoolFunc Signed-off-by: Sebastiaan van Stijn --- bool_func_go1.21_test.go | 110 +++++++++++++++++++++++++++++++++++++++ bool_func_test.go | 101 ----------------------------------- func_go1.21_test.go | 102 ++++++++++++++++++++++++++++++++++++ func_test.go | 93 --------------------------------- 4 files changed, 212 insertions(+), 194 deletions(-) create mode 100644 bool_func_go1.21_test.go create mode 100644 func_go1.21_test.go diff --git a/bool_func_go1.21_test.go b/bool_func_go1.21_test.go new file mode 100644 index 00000000..5b1bd559 --- /dev/null +++ b/bool_func_go1.21_test.go @@ -0,0 +1,110 @@ +//go:build go1.21 +// +build go1.21 + +package pflag + +import ( + "errors" + "flag" + "io" + "strings" + "testing" +) + +func TestBoolFuncCompat(t *testing.T) { + // compare behavior with the stdlib 'flag' package + type BoolFuncFlagSet interface { + BoolFunc(name string, usage string, fn func(string) error) + Parse([]string) error + } + + unitTestErr := errors.New("unit test error") + runCase := func(f BoolFuncFlagSet, name string, args []string) (values []string, err error) { + fn := func(s string) error { + values = append(values, s) + if s == "err" { + return unitTestErr + } + return nil + } + f.BoolFunc(name, "Callback function", fn) + + err = f.Parse(args) + return values, err + } + + t.Run("regular parsing", func(t *testing.T) { + flagName := "bflag" + args := []string{"--bflag", "--bflag=false", "--bflag=1", "--bflag=bar", "--bflag="} + + // It turns out that, even though the function is called "BoolFunc", + // the standard flag package does not try to parse the value assigned to + // that cli flag as a boolean. The string provided on the command line is + // passed as is to the callback. + // e.g: with "--bflag=not_a_bool" on the command line, the FlagSet does not + // generate an error stating "invalid boolean value", and `fn` will be called + // with "not_a_bool" as an argument. + + stdFSet := flag.NewFlagSet("std test", flag.ContinueOnError) + stdValues, err := runCase(stdFSet, flagName, args) + if err != nil { + t.Fatalf("std flag: expected no error, got %v", err) + } + expected := []string{"true", "false", "1", "bar", ""} + if !cmpLists(expected, stdValues) { + t.Fatalf("std flag: expected %v, got %v", expected, stdValues) + } + + fset := NewFlagSet("pflag test", ContinueOnError) + pflagValues, err := runCase(fset, flagName, args) + if err != nil { + t.Fatalf("pflag: expected no error, got %v", err) + } + if !cmpLists(stdValues, pflagValues) { + t.Fatalf("pflag: expected %v, got %v", stdValues, pflagValues) + } + }) + + t.Run("error triggered by callback", func(t *testing.T) { + flagName := "bflag" + args := []string{"--bflag", "--bflag=err", "--bflag=after"} + + // test behavior of standard flag.Fset with an error triggered by the callback: + // (note: as can be seen in 'runCase()', if the callback sees "err" as a value + // for the bool flag, it will return an error) + stdFSet := flag.NewFlagSet("std test", flag.ContinueOnError) + stdFSet.SetOutput(io.Discard) // suppress output + + // run test case with standard flag.Fset + stdValues, err := runCase(stdFSet, flagName, args) + + // double check the standard behavior: + // - .Parse() should return an error, which contains the error message + if err == nil { + t.Fatalf("std flag: expected an error triggered by callback, got no error instead") + } + if !strings.HasSuffix(err.Error(), unitTestErr.Error()) { + t.Fatalf("std flag: expected unittest error, got unexpected error value: %T %v", err, err) + } + // - the function should have been called twice, with the first two values, + // the final "=after" should not be recorded + expected := []string{"true", "err"} + if !cmpLists(expected, stdValues) { + t.Fatalf("std flag: expected %v, got %v", expected, stdValues) + } + + // now run the test case on a pflag FlagSet: + fset := NewFlagSet("pflag test", ContinueOnError) + pflagValues, err := runCase(fset, flagName, args) + + // check that there is a similar error (note: pflag will _wrap_ the error, while the stdlib + // currently keeps the original message but creates a flat errors.Error) + if !errors.Is(err, unitTestErr) { + t.Fatalf("pflag: got unexpected error value: %T %v", err, err) + } + // the callback should be called the same number of times, with the same values: + if !cmpLists(stdValues, pflagValues) { + t.Fatalf("pflag: expected %v, got %v", stdValues, pflagValues) + } + }) +} diff --git a/bool_func_test.go b/bool_func_test.go index c16be831..765c9c08 100644 --- a/bool_func_test.go +++ b/bool_func_test.go @@ -1,9 +1,6 @@ package pflag import ( - "errors" - "flag" - "io" "strings" "testing" ) @@ -48,104 +45,6 @@ func TestBoolFuncP(t *testing.T) { } } -func TestBoolFuncCompat(t *testing.T) { - // compare behavior with the stdlib 'flag' package - type BoolFuncFlagSet interface { - BoolFunc(name string, usage string, fn func(string) error) - Parse([]string) error - } - - unitTestErr := errors.New("unit test error") - runCase := func(f BoolFuncFlagSet, name string, args []string) (values []string, err error) { - fn := func(s string) error { - values = append(values, s) - if s == "err" { - return unitTestErr - } - return nil - } - f.BoolFunc(name, "Callback function", fn) - - err = f.Parse(args) - return values, err - } - - t.Run("regular parsing", func(t *testing.T) { - flagName := "bflag" - args := []string{"--bflag", "--bflag=false", "--bflag=1", "--bflag=bar", "--bflag="} - - // It turns out that, even though the function is called "BoolFunc", - // the standard flag package does not try to parse the value assigned to - // that cli flag as a boolean. The string provided on the command line is - // passed as is to the callback. - // e.g: with "--bflag=not_a_bool" on the command line, the FlagSet does not - // generate an error stating "invalid boolean value", and `fn` will be called - // with "not_a_bool" as an argument. - - stdFSet := flag.NewFlagSet("std test", flag.ContinueOnError) - stdValues, err := runCase(stdFSet, flagName, args) - if err != nil { - t.Fatalf("std flag: expected no error, got %v", err) - } - expected := []string{"true", "false", "1", "bar", ""} - if !cmpLists(expected, stdValues) { - t.Fatalf("std flag: expected %v, got %v", expected, stdValues) - } - - fset := NewFlagSet("pflag test", ContinueOnError) - pflagValues, err := runCase(fset, flagName, args) - if err != nil { - t.Fatalf("pflag: expected no error, got %v", err) - } - if !cmpLists(stdValues, pflagValues) { - t.Fatalf("pflag: expected %v, got %v", stdValues, pflagValues) - } - }) - - t.Run("error triggered by callback", func(t *testing.T) { - flagName := "bflag" - args := []string{"--bflag", "--bflag=err", "--bflag=after"} - - // test behavior of standard flag.Fset with an error triggered by the callback: - // (note: as can be seen in 'runCase()', if the callback sees "err" as a value - // for the bool flag, it will return an error) - stdFSet := flag.NewFlagSet("std test", flag.ContinueOnError) - stdFSet.SetOutput(io.Discard) // suppress output - - // run test case with standard flag.Fset - stdValues, err := runCase(stdFSet, flagName, args) - - // double check the standard behavior: - // - .Parse() should return an error, which contains the error message - if err == nil { - t.Fatalf("std flag: expected an error triggered by callback, got no error instead") - } - if !strings.HasSuffix(err.Error(), unitTestErr.Error()) { - t.Fatalf("std flag: expected unittest error, got unexpected error value: %T %v", err, err) - } - // - the function should have been called twice, with the first two values, - // the final "=after" should not be recorded - expected := []string{"true", "err"} - if !cmpLists(expected, stdValues) { - t.Fatalf("std flag: expected %v, got %v", expected, stdValues) - } - - // now run the test case on a pflag FlagSet: - fset := NewFlagSet("pflag test", ContinueOnError) - pflagValues, err := runCase(fset, flagName, args) - - // check that there is a similar error (note: pflag will _wrap_ the error, while the stdlib - // currently keeps the original message but creates a flat errors.Error) - if !errors.Is(err, unitTestErr) { - t.Fatalf("pflag: got unexpected error value: %T %v", err, err) - } - // the callback should be called the same number of times, with the same values: - if !cmpLists(stdValues, pflagValues) { - t.Fatalf("pflag: expected %v, got %v", stdValues, pflagValues) - } - }) -} - func TestBoolFuncUsage(t *testing.T) { t.Run("regular func flag", func(t *testing.T) { // regular boolfunc flag: diff --git a/func_go1.21_test.go b/func_go1.21_test.go new file mode 100644 index 00000000..2d5ea31a --- /dev/null +++ b/func_go1.21_test.go @@ -0,0 +1,102 @@ +//go:build go1.21 +// +build go1.21 + +package pflag + +import ( + "errors" + "flag" + "io" + "strings" + "testing" +) + +func TestFuncCompat(t *testing.T) { + // compare behavior with the stdlib 'flag' package + type FuncFlagSet interface { + Func(name string, usage string, fn func(string) error) + Parse([]string) error + } + + unitTestErr := errors.New("unit test error") + runCase := func(f FuncFlagSet, name string, args []string) (values []string, err error) { + fn := func(s string) error { + values = append(values, s) + if s == "err" { + return unitTestErr + } + return nil + } + f.Func(name, "Callback function", fn) + + err = f.Parse(args) + return values, err + } + + t.Run("regular parsing", func(t *testing.T) { + flagName := "fnflag" + args := []string{"--fnflag=xx", "--fnflag", "yy", "--fnflag=zz"} + + stdFSet := flag.NewFlagSet("std test", flag.ContinueOnError) + stdValues, err := runCase(stdFSet, flagName, args) + if err != nil { + t.Fatalf("std flag: expected no error, got %v", err) + } + expected := []string{"xx", "yy", "zz"} + if !cmpLists(expected, stdValues) { + t.Fatalf("std flag: expected %v, got %v", expected, stdValues) + } + + fset := NewFlagSet("pflag test", ContinueOnError) + pflagValues, err := runCase(fset, flagName, args) + if err != nil { + t.Fatalf("pflag: expected no error, got %v", err) + } + if !cmpLists(stdValues, pflagValues) { + t.Fatalf("pflag: expected %v, got %v", stdValues, pflagValues) + } + }) + + t.Run("error triggered by callback", func(t *testing.T) { + flagName := "fnflag" + args := []string{"--fnflag", "before", "--fnflag", "err", "--fnflag", "after"} + + // test behavior of standard flag.Fset with an error triggered by the callback: + // (note: as can be seen in 'runCase()', if the callback sees "err" as a value + // for the flag, it will return an error) + stdFSet := flag.NewFlagSet("std test", flag.ContinueOnError) + stdFSet.SetOutput(io.Discard) // suppress output + + // run test case with standard flag.Fset + stdValues, err := runCase(stdFSet, flagName, args) + + // double check the standard behavior: + // - .Parse() should return an error, which contains the error message + if err == nil { + t.Fatalf("std flag: expected an error triggered by callback, got no error instead") + } + if !strings.HasSuffix(err.Error(), unitTestErr.Error()) { + t.Fatalf("std flag: expected unittest error, got unexpected error value: %T %v", err, err) + } + // - the function should have been called twice, with the first two values, + // the final "=after" should not be recorded + expected := []string{"before", "err"} + if !cmpLists(expected, stdValues) { + t.Fatalf("std flag: expected %v, got %v", expected, stdValues) + } + + // now run the test case on a pflag FlagSet: + fset := NewFlagSet("pflag test", ContinueOnError) + pflagValues, err := runCase(fset, flagName, args) + + // check that there is a similar error (note: pflag will _wrap_ the error, while the stdlib + // currently keeps the original message but creates a flat errors.Error) + if !errors.Is(err, unitTestErr) { + t.Fatalf("pflag: got unexpected error value: %T %v", err, err) + } + // the callback should be called the same number of times, with the same values: + if !cmpLists(stdValues, pflagValues) { + t.Fatalf("pflag: expected %v, got %v", stdValues, pflagValues) + } + }) +} diff --git a/func_test.go b/func_test.go index d492b488..4badf936 100644 --- a/func_test.go +++ b/func_test.go @@ -1,9 +1,6 @@ package pflag import ( - "errors" - "flag" - "io" "strings" "testing" ) @@ -62,96 +59,6 @@ func TestFuncP(t *testing.T) { } } -func TestFuncCompat(t *testing.T) { - // compare behavior with the stdlib 'flag' package - type FuncFlagSet interface { - Func(name string, usage string, fn func(string) error) - Parse([]string) error - } - - unitTestErr := errors.New("unit test error") - runCase := func(f FuncFlagSet, name string, args []string) (values []string, err error) { - fn := func(s string) error { - values = append(values, s) - if s == "err" { - return unitTestErr - } - return nil - } - f.Func(name, "Callback function", fn) - - err = f.Parse(args) - return values, err - } - - t.Run("regular parsing", func(t *testing.T) { - flagName := "fnflag" - args := []string{"--fnflag=xx", "--fnflag", "yy", "--fnflag=zz"} - - stdFSet := flag.NewFlagSet("std test", flag.ContinueOnError) - stdValues, err := runCase(stdFSet, flagName, args) - if err != nil { - t.Fatalf("std flag: expected no error, got %v", err) - } - expected := []string{"xx", "yy", "zz"} - if !cmpLists(expected, stdValues) { - t.Fatalf("std flag: expected %v, got %v", expected, stdValues) - } - - fset := NewFlagSet("pflag test", ContinueOnError) - pflagValues, err := runCase(fset, flagName, args) - if err != nil { - t.Fatalf("pflag: expected no error, got %v", err) - } - if !cmpLists(stdValues, pflagValues) { - t.Fatalf("pflag: expected %v, got %v", stdValues, pflagValues) - } - }) - - t.Run("error triggered by callback", func(t *testing.T) { - flagName := "fnflag" - args := []string{"--fnflag", "before", "--fnflag", "err", "--fnflag", "after"} - - // test behavior of standard flag.Fset with an error triggered by the callback: - // (note: as can be seen in 'runCase()', if the callback sees "err" as a value - // for the flag, it will return an error) - stdFSet := flag.NewFlagSet("std test", flag.ContinueOnError) - stdFSet.SetOutput(io.Discard) // suppress output - - // run test case with standard flag.Fset - stdValues, err := runCase(stdFSet, flagName, args) - - // double check the standard behavior: - // - .Parse() should return an error, which contains the error message - if err == nil { - t.Fatalf("std flag: expected an error triggered by callback, got no error instead") - } - if !strings.HasSuffix(err.Error(), unitTestErr.Error()) { - t.Fatalf("std flag: expected unittest error, got unexpected error value: %T %v", err, err) - } - // - the function should have been called twice, with the first two values, - // the final "=after" should not be recorded - expected := []string{"before", "err"} - if !cmpLists(expected, stdValues) { - t.Fatalf("std flag: expected %v, got %v", expected, stdValues) - } - - // now run the test case on a pflag FlagSet: - fset := NewFlagSet("pflag test", ContinueOnError) - pflagValues, err := runCase(fset, flagName, args) - - // check that there is a similar error (note: pflag will _wrap_ the error, while the stdlib - // currently keeps the original message but creates a flat errors.Error) - if !errors.Is(err, unitTestErr) { - t.Fatalf("pflag: got unexpected error value: %T %v", err, err) - } - // the callback should be called the same number of times, with the same values: - if !cmpLists(stdValues, pflagValues) { - t.Fatalf("pflag: expected %v, got %v", stdValues, pflagValues) - } - }) -} - func TestFuncUsage(t *testing.T) { t.Run("regular func flag", func(t *testing.T) { // regular func flag: From 7e4dfb1e325ce429e29994933210abe53de7041d Mon Sep 17 00:00:00 2001 From: Tomas Aschan <1550920+tomasaschan@users.noreply.github.com> Date: Mon, 1 Sep 2025 21:32:25 +0200 Subject: [PATCH 3/3] Test on Go 1.12 Since 1.12 is what we specify in go.mod, and therefore implicitly is what we promise to work with, we should ensure that we don't introduce changes which break this promise (e.g. as 1bf832c6fec8a35a8c1d61e5fb14f5ce404197ef). Signed-off-by: Sebastiaan van Stijn --- .github/workflows/ci.yaml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/ci.yaml b/.github/workflows/ci.yaml index 42f7614a..7adfc939 100644 --- a/.github/workflows/ci.yaml +++ b/.github/workflows/ci.yaml @@ -13,7 +13,7 @@ jobs: strategy: fail-fast: false matrix: - go: ["1.21", "1.22", "1.23"] + go: ["1.12", "1.21", "1.22", "1.23"] steps: - name: Checkout repository