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!: Include -- in passthrough args #436

Merged

Conversation

boblail
Copy link
Contributor

@boblail boblail commented Jul 5, 2024

Given a grammar like this:

Issue

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

If Kong parses cli foo -- bar, it will populate Args with []string{"foo", "--", "bar"} (including "--"). However, if Kong parses cli -- foo bar, will populate Args with []string{"foo", "bar"} (leaving off "--").

This differs from the behavior of a passthrough Command, where "--" is included with the args in both cases.

Proposal

There are 3 places where c.endParsing() is called

  1. When node.Passthrough is true:

    kong/context.go

    Lines 366 to 368 in 5f9c5cc

    if node.Passthrough {
    c.endParsing()
    }
  2. When arg.Passthrough is true:

    kong/context.go

    Lines 451 to 453 in 5f9c5cc

    if arg.Passthrough {
    c.endParsing()
    }
  3. When "--" is encountered:

    kong/context.go

    Lines 384 to 387 in 5f9c5cc

    // Indicates end of parsing. All remaining arguments are treated as positional arguments only.
    case v == "--":
    c.scan.Pop()
    c.endParsing()

The first two do not also pop any tokens. The third one does.

This commit makes c.scan.Pop() conditional, skipping it when the next positional argument is passthrough.

Breaking Change

I believe this will cause Kong to behave a little more consistently — and from my perspective, -- is relevant for args intended to be passed through! — but it will change the behavior of existing projects that use arg:"" passthrough:"".

Given a grammar like this:

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

If Kong parses `cli foo -- bar`, it will populate `Args` with `[]string{"foo", "--", "bar"}` (including "`--`").
However, if Kong parses `cli -- foo bar`, will populate `Args` with `[]string{"foo", "bar"}` (leaving off `"--"`).

This differs from the behavior of a passthrough Command, where `"--"` is included with the args in both cases.

There are 3 places where `c.endParsing()` is called
1. When `node.Passthrough` is true: https://github.com/alecthomas/kong/blob/5f9c5cc822bdb888a3671c44d4688a6f602ecb90/context.go#L366-L368
2. When `arg.Passthrough` is true: https://github.com/alecthomas/kong/blob/5f9c5cc822bdb888a3671c44d4688a6f602ecb90/context.go#L451-L453
3. When `"--"` is encountered: https://github.com/alecthomas/kong/blob/5f9c5cc822bdb888a3671c44d4688a6f602ecb90/context.go#L384-L387

The first two do not also pop any tokens. The third one does.

This commit makes `c.scan.Pop()` conditional, skipping it when the next positional argument is passthrough.

I believe this will cause Kong to behave a little more consistently — and from my perspective, `--` is relevant for args intended to be passed through! — but it will change the behavior of existing projects that use `arg:"" passthrough:""`.
@alecthomas
Copy link
Owner

This was actually intentional for compatibility with the previous behaviour of passthrough, but I think it's okay to change it. I've been meaning to cut a 1.0 release, and this is a good motivation.

@alecthomas
Copy link
Owner

If you find any other consistencies, now would be the time.

@alecthomas alecthomas merged commit 9924ec4 into alecthomas:master Sep 11, 2024
5 checks passed
@alecthomas
Copy link
Owner

Released in v1.2.0

@AlekSi
Copy link
Contributor

AlekSi commented Sep 18, 2024

So technically, it was a breaking change in a minor release (1.1 -> 1.2). Having a changelog with a list of breaking changes would be nice.

@alecthomas
Copy link
Owner

"Technically", yes, but that was mainly because I forgot to include it in the 1.0 release. It was added very shortly after, so I'm okay with it.

This is the only breaking change.

@alecthomas
Copy link
Owner

I added a note to the README about the 1.0 release.

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.

3 participants