fix xflag delimiter and flag value parsing (#25)

Author: mfridman

xflag/parse.go

@@ -14,46 +14,38 @@ import (
 //
 // 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
-	}
+	arguments, trailingArgs := splitAtDelimiter(arguments)
 	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.
+	parseOnce := true
+	for parseOnce || len(arguments) > 0 {
+		parseOnce = false
+		// If the next argument looks like a flag, parses like a flag, and quacks like a flag,
+		// then it probably is a flag. Let the standard parser make that determination. When it
+		// instead stops at a positional argument, preserve that argument and resume parsing after
+		// it on the next iteration.
 		//
-		// If you want to treat an unknown flag as a positional argument. For example:
+		// There is one edge case here which we EXPLICITLY do not handle, and quite honestly
+		// 99.999% of the time you wouldn't build a CLI with this behavior: treating 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
+		// This triggers an error. Some users might want the unknown flag to be treated as a
+		// positional argument instead. That behavior could be added by using VisitAll to collect
+		// the defined flags before deciding whether to pass a flag-looking argument to Parse.
+		if err := f.Parse(arguments); err != nil {
+			return err
+		}
+
+		arguments = f.Args()
+		if len(arguments) == 0 {
+			break
 		}
-		args = append(args, arg)
+
+		args = append(args, arguments[0])
+		arguments = arguments[1:]
 	}
+	args = append(args, trailingArgs...)
 	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
@@ -62,3 +54,12 @@ func ParseToEnd(f *flag.FlagSet, arguments []string) error {
 	}
 	return nil
 }
+
+func splitAtDelimiter(arguments []string) (before, after []string) {
+	for i, arg := range arguments {
+		if arg == "--" {
+			return arguments[:i], arguments[i+1:]
+		}
+	}
+	return arguments, nil
+}

xflag/parse_test.go

@@ -15,6 +15,13 @@ func TestParseToEnd(t *testing.T) {
 		require.NoError(t, ParseToEnd(fs, []string{}))
 		require.False(t, *debugP)
 		require.Equal(t, 0, fs.NFlag())
+		require.True(t, fs.Parsed())
+	})
+	t.Run("only double dash terminator", func(t *testing.T) {
+		fs := flag.NewFlagSet("name", flag.ContinueOnError)
+		require.NoError(t, ParseToEnd(fs, []string{"--"}))
+		require.Empty(t, fs.Args())
+		require.True(t, fs.Parsed())
 	})
 	t.Run("no args", func(t *testing.T) {
 		fs := flag.NewFlagSet("name", flag.ContinueOnError)
@@ -115,6 +122,11 @@ func TestParseToEnd(t *testing.T) {
 		require.Error(t, err)
 		require.Equal(t, err.Error(), "flag provided but not defined: -some-unknown-flag")
 	})
+	t.Run("missing flag value after positional argument", func(t *testing.T) {
+		fs, _ := newFlagset()
+		err := ParseToEnd(fs, []string{"arg1", "--flag1"})
+		require.EqualError(t, err, "flag needs an argument: -flag1")
+	})
 	t.Run("only positional args", func(t *testing.T) {
 		fs, c := newFlagset()
 		err := ParseToEnd(fs, []string{"arg1", "arg2", "arg3"})
@@ -152,6 +164,14 @@ func TestParseToEnd(t *testing.T) {
 		require.Equal(t, "value2", c.flag2)
 		require.Equal(t, []string{"arg1", "arg2", "arg3"}, fs.Args())
 	})
+	t.Run("flag-looking value after positional argument", func(t *testing.T) {
+		fs, c := newFlagset()
+		err := ParseToEnd(fs, []string{"arg1", "--flag1", "--flag3", "arg2"})
+		require.NoError(t, err)
+		require.Equal(t, "--flag3", c.flag1)
+		require.False(t, c.flag3)
+		require.Equal(t, []string{"arg1", "arg2"}, fs.Args())
+	})
 	t.Run("standalone dash is positional", func(t *testing.T) {
 		fs, c := newFlagset()
 		args := []string{"--flag1=value1", "-", "arg1"}
@@ -162,15 +182,32 @@ func TestParseToEnd(t *testing.T) {
 	})
 	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())
+		require.False(t, c.flag3)
+		require.Equal(t, []string{"--flag3"}, fs.Args())
+	})
+	t.Run("double dash terminator before flags", func(t *testing.T) {
+		fs, c := newFlagset()
+		err := ParseToEnd(fs, []string{"--", "--flag3"})
+		require.NoError(t, err)
+		require.False(t, c.flag3)
+		require.Equal(t, []string{"--flag3"}, fs.Args())
+	})
+	t.Run("unknown flag after double dash terminator", func(t *testing.T) {
+		fs, _ := newFlagset()
+		err := ParseToEnd(fs, []string{"arg1", "--", "--unknown", "arg2"})
+		require.NoError(t, err)
+		require.Equal(t, []string{"arg1", "--unknown", "arg2"}, fs.Args())
+	})
+	t.Run("second double dash is positional", func(t *testing.T) {
+		fs, c := newFlagset()
+		err := ParseToEnd(fs, []string{"--", "--", "--flag3"})
+		require.NoError(t, err)
+		require.False(t, c.flag3)
+		require.Equal(t, []string{"--", "--flag3"}, fs.Args())
 	})
 	t.Run("duplicate flags last wins", func(t *testing.T) {
 		fs, c := newFlagset()