diff --git a/go.mod b/go.mod index 8a81c3f..9912a8e 100644 --- a/go.mod +++ b/go.mod @@ -2,10 +2,7 @@ module github.com/pressly/cli go 1.21.0 -require ( - github.com/mfridman/xflag v0.1.0 - github.com/stretchr/testify v1.11.1 -) +require github.com/stretchr/testify v1.11.1 require ( github.com/davecgh/go-spew v1.1.1 // indirect diff --git a/go.sum b/go.sum index 3da4147..c4c1710 100644 --- a/go.sum +++ b/go.sum @@ -1,7 +1,5 @@ github.com/davecgh/go-spew v1.1.1 h1:vj9j/u1bqnvCEfJOwUhtlOARqs3+rkHYY13jYWTU97c= github.com/davecgh/go-spew v1.1.1/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38= -github.com/mfridman/xflag v0.1.0 h1:TWZrZwG1QklFX5S4j1vxfF1sZbZeZSGofMwPMLAF29M= -github.com/mfridman/xflag v0.1.0/go.mod h1:/483ywM5ZO5SuMVjrIGquYNE5CzLrj5Ux/LxWWnjRaE= github.com/pmezard/go-difflib v1.0.0 h1:4DBwDE0NGyQoBHbLQYPwSUPoCMWR5BEzIk/f1lZbAQM= github.com/pmezard/go-difflib v1.0.0/go.mod h1:iKH77koFhYxTK1pcRnkKkqfTogsbg7gZNVY4sRDYZ/4= github.com/stretchr/testify v1.11.1 h1:7s2iGBzp5EwR7/aIZr8ao5+dra3wiQyKjjFuvgVKu7U= diff --git a/parse.go b/parse.go index 98f2f4e..d892e67 100644 --- a/parse.go +++ b/parse.go @@ -10,7 +10,7 @@ import ( "strconv" "strings" - "github.com/mfridman/xflag" + "github.com/pressly/cli/xflag" ) // Parse traverses the command hierarchy and parses arguments. It returns an error if parsing fails diff --git a/xflag/doc.go b/xflag/doc.go new file mode 100644 index 0000000..2a0821d --- /dev/null +++ b/xflag/doc.go @@ -0,0 +1,5 @@ +// Package xflag extends the standard library's flag package to support parsing flags interleaved +// with positional arguments. By default, Go's flag package stops parsing flags at the first +// non-flag argument, which is unintuitive for most CLI users. This package provides [ParseToEnd] as +// a drop-in replacement that handles flags anywhere in the argument list. +package xflag diff --git a/xflag/parse.go b/xflag/parse.go new file mode 100644 index 0000000..7284d76 --- /dev/null +++ b/xflag/parse.go @@ -0,0 +1,64 @@ +package xflag + +import ( + "flag" +) + +// ParseToEnd is a drop-in replacement for flag.Parse. It improves upon the standard behavior by +// parsing flags even when they are interspersed with positional arguments. This overcomes Go's +// default limitation of stopping flag parsing upon encountering the first positional argument. For +// more details, see: +// +// - https://github.com/golang/go/issues/4513 +// - https://github.com/golang/go/issues/63138 +// +// This is a bit unfortunate, but most users nowadays consuming CLI tools expect this behavior. +func ParseToEnd(f *flag.FlagSet, arguments []string) error { + if err := f.Parse(arguments); err != nil { + return err + } + if f.NArg() == 0 { + return nil + } + var args []string + remainingArgs := f.Args() + for i := 0; i < len(remainingArgs); i++ { + arg := remainingArgs[i] + // If the arg looks like a flag, parses like a flag, and quacks like a flag, then it + // probably is a flag. + // + // Note, there's an edge cases here which we EXPLICITLY do not handle, and quite honestly + // 99.999% of the time you wouldn't build a CLI with this behavior. + // + // If you want to treat an unknown flag as a positional argument. For example: + // + // $ ./cmd --valid=true arg1 --unknown-flag=foo arg2 + // + // Right now, this will trigger an error. But *some* users might want that unknown flag to + // be treated as a positional argument. It's trivial to add this behavior, by using VisitAll + // to iterate over all defined flags (regardless if they are set), and then checking if the + // flag is in the map of known flags. + if len(arg) > 1 && arg[0] == '-' { + // If we encounter a "--", treat all subsequent arguments as positional. The "--" itself + // is stripped, consistent with the standard library's behavior. + if arg == "--" { + args = append(args, remainingArgs[i+1:]...) + break + } + if err := f.Parse(remainingArgs[i:]); err != nil { + return err + } + remainingArgs = f.Args() + i = -1 // Reset to handle newly parsed arguments. + continue + } + args = append(args, arg) + } + if len(args) > 0 { + // Use "--" as a sentinel to set the FlagSet's internal args field without unsafe + // reflection. When flag.Parse encounters "--" it stops processing and stores the remaining + // arguments as positional args, which is exactly what we need. + return f.Parse(append([]string{"--"}, args...)) + } + return nil +} diff --git a/xflag/parse_test.go b/xflag/parse_test.go new file mode 100644 index 0000000..a58a4c7 --- /dev/null +++ b/xflag/parse_test.go @@ -0,0 +1,206 @@ +package xflag + +import ( + "flag" + "io" + "testing" + + "github.com/stretchr/testify/require" +) + +func TestParseToEnd(t *testing.T) { + t.Run("empty", func(t *testing.T) { + fs := flag.NewFlagSet("name", flag.ContinueOnError) + debugP := fs.Bool("debug", false, "debug mode") + require.NoError(t, ParseToEnd(fs, []string{})) + require.False(t, *debugP) + require.Equal(t, 0, fs.NFlag()) + }) + t.Run("no args", func(t *testing.T) { + fs := flag.NewFlagSet("name", flag.ContinueOnError) + debugP := fs.Bool("debug", false, "debug mode") + err := ParseToEnd(fs, []string{"--debug", "true"}) + require.NoError(t, err) + require.Equal(t, 1, fs.NFlag()) + require.True(t, *debugP) + }) + t.Run("before with args", func(t *testing.T) { + fs := flag.NewFlagSet("name", flag.ContinueOnError) + debugP := fs.Bool("debug", false, "debug mode") + err := ParseToEnd(fs, []string{"--debug=true", "arg1", "arg2"}) + require.NoError(t, err) + require.True(t, *debugP) + require.Equal(t, 1, fs.NFlag()) + require.Equal(t, []string{"arg1", "arg2"}, fs.Args()) + }) + t.Run("after with args", func(t *testing.T) { + fs := flag.NewFlagSet("name", flag.ContinueOnError) + debugP := fs.Bool("debug", false, "debug mode") + err := ParseToEnd(fs, []string{"arg1", "arg2", "--debug"}) + require.NoError(t, err) + require.True(t, *debugP) + + f := fs.Lookup("debug") + require.Equal(t, "true", f.Value.String()) + require.Equal(t, 1, fs.NFlag()) + require.Equal(t, []string{"arg1", "arg2"}, fs.Args()) + }) + t.Run("before and after with args", func(t *testing.T) { + fs, c := newFlagset() + args := []string{ + "--flag1=value1", + "--flag3=true", + "arg1", + "arg2", + "--flag2=value2", + "--flag4=false", + "arg3", + } + err := ParseToEnd(fs, args) + require.NoError(t, err) + require.Equal(t, config{ + flag1: "value1", + flag3: true, + flag2: "value2", + flag4: false, + }, *c) + require.Equal(t, []string{"arg1", "arg2", "arg3"}, fs.Args()) + require.Equal(t, 4, fs.NFlag()) + }) + t.Run("break", func(t *testing.T) { + fs, c := newFlagset() + args := []string{ + "--flag1=value1", + "--flag3=true", + "arg1", + "arg2", + "--flag2=value2", + "--flag4=false", + "arg3", + "--", + "arg4", + "arg5", + "--flag4=true", // This is now a positional argument no matter what. + } + err := ParseToEnd(fs, args) + require.NoError(t, err) + require.Equal(t, config{ + flag1: "value1", + flag3: true, + flag2: "value2", + flag4: false, + }, *c) + require.Equal(t, []string{"arg1", "arg2", "arg3", "arg4", "arg5", "--flag4=true"}, fs.Args()) + require.Equal(t, 4, fs.NFlag()) + }) + t.Run("unknown flag before", func(t *testing.T) { + fs, _ := newFlagset() + args := []string{ + "--flag1=value1", + "--some-unknown-flag=foo", // This gets treated as a flag. + "arg1", + } + err := ParseToEnd(fs, args) + require.Error(t, err) + require.Equal(t, err.Error(), "flag provided but not defined: -some-unknown-flag") + }) + t.Run("unknown flag after", func(t *testing.T) { + fs, _ := newFlagset() + args := []string{ + "--flag1=value1", + "arg1", + "--some-unknown-flag=foo", // This gets treated as a flag. + } + err := ParseToEnd(fs, args) + require.Error(t, err) + require.Equal(t, err.Error(), "flag provided but not defined: -some-unknown-flag") + }) + t.Run("only positional args", func(t *testing.T) { + fs, c := newFlagset() + err := ParseToEnd(fs, []string{"arg1", "arg2", "arg3"}) + require.NoError(t, err) + // All flags should retain defaults. + require.Equal(t, config{flag1: "asdf", flag2: "qwerty", flag3: false, flag4: true}, *c) + require.Equal(t, 0, fs.NFlag()) + require.Equal(t, []string{"arg1", "arg2", "arg3"}, fs.Args()) + }) + t.Run("single dash flag syntax", func(t *testing.T) { + fs, c := newFlagset() + args := []string{ + "-flag1=value1", + "arg1", + "-flag3", + "arg2", + } + err := ParseToEnd(fs, args) + require.NoError(t, err) + require.Equal(t, "value1", c.flag1) + require.True(t, c.flag3) + require.Equal(t, []string{"arg1", "arg2"}, fs.Args()) + }) + t.Run("space separated flags interleaved", func(t *testing.T) { + fs, c := newFlagset() + args := []string{ + "arg1", + "--flag1", "value1", + "arg2", + "--flag2", "value2", + "arg3", + } + err := ParseToEnd(fs, args) + require.NoError(t, err) + require.Equal(t, "value1", c.flag1) + require.Equal(t, "value2", c.flag2) + require.Equal(t, []string{"arg1", "arg2", "arg3"}, fs.Args()) + }) + t.Run("standalone dash is positional", func(t *testing.T) { + fs, c := newFlagset() + args := []string{"--flag1=value1", "-", "arg1"} + err := ParseToEnd(fs, args) + require.NoError(t, err) + require.Equal(t, "value1", c.flag1) + require.Equal(t, []string{"-", "arg1"}, fs.Args()) + }) + t.Run("flags after double dash terminator", func(t *testing.T) { + fs, c := newFlagset() + // The initial f.Parse consumes --flag1 and stops at "--", leaving ["--flag3"] as remaining + // args (the "--" is stripped by std lib). The loop then parses --flag3 as a flag, + // collecting zero positional args. + args := []string{"--flag1=value1", "--", "--flag3"} + err := ParseToEnd(fs, args) + require.NoError(t, err) + require.Equal(t, "value1", c.flag1) + require.True(t, c.flag3) + require.Equal(t, 0, fs.NArg()) + }) + t.Run("duplicate flags last wins", func(t *testing.T) { + fs, c := newFlagset() + args := []string{ + "--flag1=first", + "arg1", + "--flag1=second", + } + err := ParseToEnd(fs, args) + require.NoError(t, err) + require.Equal(t, "second", c.flag1) + require.Equal(t, []string{"arg1"}, fs.Args()) + }) +} + +type config struct { + flag1 string + flag2 string + flag3 bool + flag4 bool +} + +func newFlagset() (*flag.FlagSet, *config) { + fs := flag.NewFlagSet("name", flag.ContinueOnError) + fs.SetOutput(io.Discard) + c := new(config) + fs.StringVar(&c.flag1, "flag1", "asdf", "flag1 usage") + fs.StringVar(&c.flag2, "flag2", "qwerty", "flag2 usage") + fs.BoolVar(&c.flag3, "flag3", false, "flag3 usage") + fs.BoolVar(&c.flag4, "flag4", true, "flag4 urage") + return fs, c +}