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 PassAfterNonOption not working with positional args #211

Merged
merged 3 commits into from
Sep 23, 2018

Conversation

hchargois
Copy link
Contributor

This makes PassAfterNonOption work correctly (the same as PassDoubleDash), when used with positional args (as opposed to "unspecified" args being returned by Parse).

I'm not sure this is clear enough, so let me give examples:

type Options1 struct {
    Value bool `short:"v"`
}
type Options2 struct {
    Value bool `short:"v"`
    Positional struct {
        Rest []string `required:"yes"`
    } `positional-args:"yes"`
}
Input line Using Options1, ParseArgs()... Using Options2, ParseArgs()...
-v -- -g parses -v, returns ["-g"] parses -v and sets Rest to ["-g"], returns []
-v arg -g parses -v, returns ["args", "-g"] tries to parse -g and fails ❗

I argue that this bottom-right case is wrong, it should instead not try to parse -g but set Rest to ["arg", "-g"]. This PR fixes this.

@jessevdk
Copy link
Owner

I think this change makes sense, my only worry is that it changes the behavior of commands, which previously would still apply even if PassAfterNonOption was set. I do think the behavior proposed in this PR is the correct one, I just wonder if this will break anyone's working assumptions.

@zimmski: maybe it's time for:

  1. Create v1.0.0 before merging this PR (should fix Please consider SemVer 2.0.0 versioning #206)
  2. Merge this PR, create v1.1.0 (this is more of an ABI break in a sense, but semver doesn't cover this afaik)
  3. Start writing simple release notes, and mention this change in the one for v1.1.0

@@ -649,23 +657,7 @@ func (p *Parser) parseNonOption(s *parseState) error {
}
}

if (p.Options & PassAfterNonOption) != None {
Copy link
Owner

Choose a reason for hiding this comment

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

I prefer if this stayed in this method but was just moved up.

@highbass
Copy link

Hey Guys,

What happened with this proposed change? i am running into similar issues and my current workaround is just to check if args length > 0 ...

@jessevdk jessevdk merged commit ebe2690 into jessevdk:master Sep 23, 2018
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