search

alecthomas / kong

Kong is a command-line parser for Go
MIT License
2.15k stars 140 forks source link

fix!: Include `--` in passthrough args #436

Closed boblail closed 2 months ago

boblail commented 4 months ago

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: 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.

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:"".

alecthomas commented 4 months ago

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 commented 4 months ago

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

alecthomas commented 2 months ago

Released in v1.2.0

AlekSi commented 2 months ago

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 commented 2 months ago

"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 commented 2 months ago

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