spec: parse remainder operands after --

[PATCH go-opt] spec: parse remainder operands after -- Export this patch

Parse operands after the option delimiter ("--") and store them in the
remainder ("...") struct field. If no operand is provided after the
delimiter, issue a missingOperand error. Add tests for string remainder
and string slice remainder.

Example:

Using this Foo struct

	type Foo struct {
		Delay time.Duration `opt:"-t,--delay" action:"ParseDelay" default:"1s"`
		Force bool          `opt:"--force"`
		Name  string        `opt:"name" required:"false" metavar:"FOO"`
		Cmd   []string      `opt:"..."`
	}

and the following cmdline, we would currently get this result:

$ foo --force -- bar baz 'xy z'
main.Foo{Delay:1000000000, Force:true, Name:"--", Cmd:[]string{"bar", "baz", "xy z"}}

However, with this patch applied, we get:

$ foo --force -- bar baz 'xy z'
main.Foo{Delay:1000000000, Force:true, Name:"", Cmd:[]string{"bar", "baz", "xy z"}}

Signed-off-by: Koni Marti <koni.marti@gmail.com>
---
 spec.go      |  32 ++++++++++++++
 spec_test.go | 123 +++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 155 insertions(+)

Robin Jarry <robin@jarry.cc>

7 months ago

Hi Koni,

Thanks for your patch!

I am thinking of several reasons not to apply this:

The main use case for -- is precisely for this: allowing to take --flag 
or -f as **positional arguments** even if they are valid optional 
arguments for that program.

At the moment, go-opt does not deal with this and it would be great if 
it did.

If I understand correctly, you are using -- for a slightly different use 
case. This would be very confusing to users who are used to getopt 
default behaviour. It seems like it would be best to follow that 
standard.

Koni Marti <koni.marti@gmail.com>

7 months ago

No, same use case. The example in the commit message was not the best
pick, but I just used the one from the README without more thought.

And I put the arguments after -- into the remainder ("...") with the
idea that go-opt allows to use default values for the positional
arguments. But as you pointed out that's probably more confusing than
helpful.

Happy to send a v2 where the arguments after -- are filled up in the
positional variables and with a better example in the commit, if that's
OK?

Side note: motivation for this in aerc would be the notmuch filter
queries like ':filter -- -tag:unread'. '-tag:unread' is short for 'not
tag:unread'.  If you run ':filter -tag:unread' now, it would parse '-t'
as the option flag for the To field where it'd store "ag:unread" as the
value.

Also, to achieve what you want, changing the Name optional+positional 
argument into a flag would be the best course of action.

Thoughts?

diff --git a/spec.go b/spec.go
index 323dffa..6e3486b 100644
--- a/spec.go
+++ b/spec.go
@@ -82,6 +82,8 @@ var (
 	positionalRe = regexp.MustCompile(`^([a-zA-Z][\w-]*)$`)
 )
 
+const optionDelim = "--"
+
 // Interpret all struct fields to a list of option specs
 func NewCmdSpec(name string, v any) *CmdSpec {
 	typ := reflect.TypeOf(v)
@@ -337,6 +339,15 @@ func (s *optSpec) Flag() string {
 	return usage
 }
 
+func (s *optSpec) isRemainder() bool {
+	switch s.kind {
+	case remainder, remainderSplit:
+		return true
+	default:
+		return false
+	}
+}
+
 func (c *CmdSpec) Usage() string {
 	if c.passthrough {
 		return c.name + " ..."
@@ -355,6 +366,7 @@ type errKind int
 const (
 	unknownErr errKind = iota
 	missingValue
+	missingOperand
 	takesNoValue
 	unknownFlag
 	unexpectedArg
@@ -372,6 +384,8 @@ func (e *ArgError) Error() string {
 	switch e.kind {
 	case missingValue:
 		return fmt.Sprintf("%s takes a value", e.spec.Flag())
+	case missingOperand:
+		return fmt.Sprintf("missing operand after option delimiter: %#v", e)
 	case takesNoValue:
 		return fmt.Sprintf("%s does not take a value", e.spec.Flag())
 	case unknownFlag:
@@ -486,14 +500,32 @@ func (c *CmdSpec) parseArgs(args *Args) []*ArgError {
 				cur = nil
 				goto next
 			}
+
+			isDelim := arg == optionDelim
 			for len(positionals) > 0 {
 				spec := &c.opts[positionals[0]]
 				positionals = positionals[1:]
 				if spec.appliesToAlias(c.name) {
+					if isDelim && !spec.isRemainder() {
+						// if we encounter an option
+						// delimiter, skip forward to
+						// the remainder field
+						continue
+					}
 					cur = c.markSeen(spec, i)
 					break
 				}
 			}
+
+			if isDelim {
+				args.Shift(1) // consume option delimiter
+				if args.Count() == 0 {
+					fail(missingOperand, nil, errors.New(arg))
+					goto next
+				}
+				arg = args.Arg(0)
+			}
+
 			if cur == nil {
 				fail(unexpectedArg, nil, errors.New(arg))
 				goto next
diff --git a/spec_test.go b/spec_test.go
index 1d0b6de..5c3510f 100644
--- a/spec_test.go
+++ b/spec_test.go
@@ -40,6 +40,7 @@ func TestArgsToStructErrors(t *testing.T) {
 		{"foo -z", `"-z" unexpected argument`},
 		{"bar -j4 foo baz", `"baz" unexpected argument`},
 		{"bar -j4 invalid", `invalid value`},
+		{"bar -j4 --", `missing operand`},
 	}
 
 	for _, v := range vectors {
@@ -102,3 +103,125 @@ func TestArgsToStruct(t *testing.T) {
 		})
 	}
 }
+
+type OptionRemainderStruct struct {
+	Jobs int    `opt:"-j" default:"3"`
+	Name string `opt:"name" default:"no name"`
+	Rest string `opt:"..." required:"false"`
+}
+
+func TestArgsToRemainderStruct(t *testing.T) {
+	vectors := []struct {
+		cmdline  string
+		expected OptionRemainderStruct
+	}{
+		{
+			cmdline: `bar -j4 'f o o \(°</ f o o'`,
+			expected: OptionRemainderStruct{
+				Jobs: 4,
+				Name: `f o o \(°</ f o o`,
+			},
+		},
+		{
+			cmdline: `bar 'n a m e' -j7 this is the rest`,
+			expected: OptionRemainderStruct{
+				Jobs: 7,
+				Name: "n a m e",
+				Rest: "this is the rest",
+			},
+		},
+		{
+			cmdline: `bar 'n a m e' -j7 -- this is the rest`,
+			expected: OptionRemainderStruct{
+				Jobs: 7,
+				Name: "n a m e",
+				Rest: "this is the rest",
+			},
+		},
+		{
+			cmdline: `bar 'n a m e' -j7 -- -j8`,
+			expected: OptionRemainderStruct{
+				Jobs: 7,
+				Name: "n a m e",
+				Rest: "-j8",
+			},
+		},
+		{
+			cmdline: `bar 'n a m e' -- -j8`,
+			expected: OptionRemainderStruct{
+				Jobs: 3,
+				Name: "n a m e",
+				Rest: "-j8",
+			},
+		},
+		{
+			cmdline: `bar -j4 -- 8`,
+			expected: OptionRemainderStruct{
+				Jobs: 4,
+				Name: "no name",
+				Rest: "8",
+			},
+		},
+	}
+
+	for _, v := range vectors {
+		t.Run(v.cmdline, func(t *testing.T) {
+			var s OptionRemainderStruct
+			assert.Nil(t, opt.CmdlineToStruct(v.cmdline, &s))
+			assert.Equal(t, v.expected, s)
+		})
+	}
+}
+
+type OptionRemainderSplitStruct struct {
+	Jobs int      `opt:"-j,--jobs" default:"3"`
+	Name string   `opt:"name"`
+	Rest []string `opt:"..." required:"false"`
+}
+
+func TestArgsToRemainderSplitStruct(t *testing.T) {
+	vectors := []struct {
+		cmdline  string
+		expected OptionRemainderSplitStruct
+	}{
+		{
+			cmdline: `bar --jobs=4 'f o o \(°</ f o o'`,
+			expected: OptionRemainderSplitStruct{
+				Jobs: 4,
+				Name: `f o o \(°</ f o o`,
+			},
+		},
+		{
+			cmdline: `bar 'n a m e' -j7 this is the rest`,
+			expected: OptionRemainderSplitStruct{
+				Jobs: 7,
+				Name: "n a m e",
+				Rest: []string{"this", "is", "the", "rest"},
+			},
+		},
+		{
+			cmdline: `bar 'n a m e' --jobs=7 -- -j8`,
+			expected: OptionRemainderSplitStruct{
+				Jobs: 7,
+				Name: "n a m e",
+				Rest: []string{"-j8"},
+			},
+		},
+		{
+			cmdline: `bar 'n a m e' -- -j8`,
+			expected: OptionRemainderSplitStruct{
+				Jobs: 3,
+				Name: "n a m e",
+				Rest: []string{"-j8"},
+			},
+		},
+	}
+
+	for _, v := range vectors {
+		t.Run(v.cmdline, func(t *testing.T) {
+			var s OptionRemainderSplitStruct
+			assert.Nil(t, opt.CmdlineToStruct(v.cmdline, &s))
+			assert.Equal(t, v.expected, s)
+		})
+	}
+}
-- 
2.43.0

Robin Jarry <robin@jarry.cc>

7 months ago
Koni Marti, Feb 01, 2024 at 23:49:

~rjarry/public-inbox

go-opt: spec: parse remainder operands after -- v1 PROPOSED

[PATCH go-opt] spec: parse remainder operands after -- Export this patch