fix!: Include -- in passthrough args#436
Merged
alecthomas merged 1 commit intoSep 11, 2024
Merged
Conversation
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:""`.
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. |
Owner
|
If you find any other consistencies, now would be the time. |
Owner
|
Released in v1.2.0 |
Contributor
|
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. |
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. |
Owner
|
I added a note to the README about the 1.0 release. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Given a grammar like this:
Issue
If Kong parses
cli foo -- bar, it will populateArgswith[]string{"foo", "--", "bar"}(including "--"). However, if Kong parsescli -- foo bar, will populateArgswith[]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 callednode.Passthroughis true:kong/context.go
Lines 366 to 368 in 5f9c5cc
arg.Passthroughis true:kong/context.go
Lines 451 to 453 in 5f9c5cc
"--"is encountered:kong/context.go
Lines 384 to 387 in 5f9c5cc
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 usearg:"" passthrough:"".