Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: When a Grammar combines flags with passthrough args, see if an unrecognized flag may be treated as a positional argument #435

Conversation

boblail
Copy link
Contributor

@boblail boblail commented Jul 3, 2024

Issue

Given a grammar like this:

var cli struct {
        Args []string `arg:"" optional:"" passthrough:""`
}

The first positional argument implies that it was preceded by --, so subsequent flags are not parsed.

If Kong parses cli 1 --unknown 3, it will populate Args with []string{"1", "--unknown", "3"}.
However, if Kong parses cli --unknown 2 3, it will fail saying that --unknown is an unrecognized flag.

Proposal

This commit changes the parser so that if an unknown flag could be treated as the first passthrough argument, it is.

After this change, if Kong parses cli --unknown 2 3, it will populate Args with []string{"--unknown", "2", "3"}.

context.go Outdated Show resolved Hide resolved
…nrecognized flag may be treated as a positional argument

Given a grammar like this:
```golang
var cli struct {
	Args []string `arg:"" optional:"" passthrough:""`
}
```

The first positional argument implies that it was preceded by `--`, so subsequent flags are not parsed.

If Kong parses `cli 1 --unknown 3`, it will populate `Args` with `[]string{"1", "--unknown", "3"}`.
However, if Kong parses `cli --unknown 2 3`, it will fail saying that `--unknown` is an unrecognized flag.

This commit changes the parser so that if an unknown flag _could_ be treated as the first passthrough argument, it is.

After this change, if Kong parses `cli --unknown 2 3`, it will populate `Args` with `[]string{"--unknown", "2", "3"}`.
@boblail boblail force-pushed the lail/passthrough-args-accepts-unrecognized-flags branch from be56a49 to 5aa7f97 Compare July 5, 2024 04:48
Comment on lines +90 to +93

- linters: [maintidx]
path: context\.go
text: 'Function name: trace'
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To bypass the linter error:

context.go:350:1: Function name: trace, Cyclomatic Complexity: 47, Halstead Volume: 4627.00, Maintainability Index: 18 (maintidx)

Would you prefer I extracted something from trace instead? Or reworked it to not have a nested if?

... maybe like

case FlagToken:
	if err := c.parseFlag(flags, token.String()); err == nil {
		// Flag was parsed successfully.
	} else if isUnknownFlagError(err) && positional < len(node.Positional) && node.Positional[positional].Passthrough {
		c.scan.Pop()
		c.scan.PushTyped(token.String(), PositionalArgumentToken)
	} else {
		return err
	}

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's fine, the linters are just guides, I usually disable them as necessary. FYI you can also just use //nolint:maintidx on the offending line.

@alecthomas alecthomas merged commit fcb5e05 into alecthomas:master Jul 5, 2024
5 checks passed
@alecthomas
Copy link
Owner

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants