~rjarry/public-inbox

This thread contains a patchset. You're looking at the original emails, but you may wish to use the patch review UI. Review patch
2 2

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

Details
Message ID
<20240201224858.457986-1-koni.marti@gmail.com>
DKIM signature
pass
Download raw message
Patch: +155 -0
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(+)

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
Details
Message ID
<CYVMYZGPS6DA.21DTWU7DV7QR6@ringo>
In-Reply-To
<20240201224858.457986-1-koni.marti@gmail.com> (view parent)
DKIM signature
pass
Download raw message
Koni Marti, Feb 01, 2024 at 23:49:
> 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(+)

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.

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

Thoughts?
Details
Message ID
<CYVS0R28UACA.20ZAG0NVGEZ2D@gmail.com>
In-Reply-To
<CYVMYZGPS6DA.21DTWU7DV7QR6@ringo> (view parent)
DKIM signature
pass
Download raw message
On Sat Feb 3, 2024 at 6:56 PM CET, Robin Jarry wrote:
> 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.

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.

-- 
Koni
Reply to thread Export thread (mbox)