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

Positional arguments with TrailingVarArgs shouldn't split on delim #511

Closed
luser opened this issue May 22, 2016 · 4 comments
Closed

Positional arguments with TrailingVarArgs shouldn't split on delim #511

luser opened this issue May 22, 2016 · 4 comments
Labels
A-parsing Area: Parser's logic and needs it changed somehow.
Milestone

Comments

@luser
Copy link

luser commented May 22, 2016

I'm using AppSettings::TrailingVarArg for a binary that wraps compiler execution to pass the rest of the commandline arguments down to the compiler. I found that compiler arguments were unexpectedly being split on commas (like -Wl,-foo => ["-Wl", "-foo"]).

I wrote a (failing) unit test for this, but haven't tried to actually fix it:
luser@ad9f8d8

parse_positional calls add_val_to_arg:
https://github.com/kbknapp/clap-rs/blob/c8c20a046e9c2ea187fe2f511d5e2326d154025b/src/app/macros.rs#L122

which checks val_delim:
https://github.com/kbknapp/clap-rs/blob/1de71c00585043758608f096e256c03666513a16/src/app/parser.rs#L1119

...which defaults to ",":
https://github.com/kbknapp/clap-rs/blob/1de71c00585043758608f096e256c03666513a16/src/args/arg.rs#L98

@kbknapp
Copy link
Member

kbknapp commented May 23, 2016

Thanks for taking the time to find all this!

Since the AppSettings::TrailingVarArg requires a positional argument to work, one could set Arg::use_delimiter(false) for that final positional argument. This allows the end user to decide whether or not they want to delimit those values or not.

If you set that to false does it still behave undesirably? If so, this is a bug and should be an easy fix.

The other option is to make the default behavior, if AppSettings::TrailingVarArg is set, to not use delimiters but I'd be curious to know about more use cases first.

@kbknapp kbknapp added T: RFC / question A-parsing Area: Parser's logic and needs it changed somehow. labels May 23, 2016
@luser
Copy link
Author

luser commented May 23, 2016

I was able to work around this by explicitly calling use_delimiter(false) on the positional argument:
https://github.com/luser/sccache2/blob/254a738e1cac3e40cd5a8ea0f74715a11293995d/src/cmdline.rs#L46

I still think this is undesirable behavior for this case. Consider that the same thing manifests in the case where "--" is passed to delineate a separate set of arguments, as in this other failing unittest:
https://github.com/luser/clap-rs/blob/b92045ce9d0090ffcbeb124465653eb3c711a7ef/tests/positionals.rs#L18

@kbknapp
Copy link
Member

kbknapp commented May 23, 2016

What we could do is add some logic to check if either AppSettings::TrailingVarArg or -- has been passed and stop delimiting values on that final positional argument if so, otherwise continue delimiting values.

My only hesitation with doing that is, is there anyone who wants values to be delimited when either of those two conditions are met? I'd say clap is small enough that the likelihood is improbable, but it's something I'd like to consider. Primarily because making this change would also preclude anyone from being able to delimit values in those conditions if they truly wanted to.

@luser
Copy link
Author

luser commented Jun 7, 2016

I can't imagine what the use case would be for that behavior, but it's possible someone would want it, I guess? Personally I think the delimiter-splitting shouldn't apply to positional arguments at all, that's very unintuitive to me. Supporting --foo=abc,xyz => "foo": ["abc", "xyz"] makes sense, but foo abc,xyz => ["foo", "abc", "xyz"] is surprising.

@kbknapp kbknapp added this to the 2.6.0 milestone Jun 8, 2016
@homu homu closed this as completed in fc3e0f5 Jun 8, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-parsing Area: Parser's logic and needs it changed somehow.
Projects
None yet
Development

No branches or pull requests

2 participants