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

allow app names, options and subcommands with hyphens when using the macro #321

Closed
flying-sheep opened this issue Oct 27, 2015 · 19 comments
Closed
Labels
C-enhancement Category: Raise on the bar on expectations S-waiting-on-decision Status: Waiting on a go/no-go before implementing

Comments

@flying-sheep
Copy link
Contributor

the following code creates a -b/--group option, not a -g/--group-by option.

clap_app!(myapp =>
    ...
    (@arg group_by: -g --group-by +takes_value "foo")
)
@flying-sheep flying-sheep changed the title allow options and subcommands with hyphens allow app names, options and subcommands with hyphens Oct 27, 2015
@kbknapp
Copy link
Member

kbknapp commented Oct 27, 2015

@flying-sheep the macro is still a work in progress, so thanks for reporting this! @james-darkfox ideas?

As a hacky work-around until this bug is fixed, you should be able to use a combination of the macro and traditional builder pattern (and builder pattern is just as fast as macro performance wise, just more verbose):

let app = clap_app!(myapp =>
    ...
    (@arg group_by: -g --group-by +takes_value "foo")
)
.arg(Arg::with_name("group_by")
    .short("g")
    .long("group-by")
    .takes_value(true)
    .help("foo"));

@kbknapp kbknapp added C-bug Category: Updating dependencies P3: want to have and removed T: RFC / question labels Oct 27, 2015
@flying-sheep
Copy link
Contributor Author

ha, i think for a one-time construction like here, any performance is good enough 😄

@kbknapp
Copy link
Member

kbknapp commented Oct 28, 2015

If parsing speed isn't a concern you can achieve a similarly less verbose using the "usage style." This isn't to say the usage strings are slow, they're still quite fast, just not quite as fast as the builder pattern (or macro which simply expands to the builder).

.arg_from_usage("-g, --group-by [group_by] 'foo'")

@kbknapp kbknapp added this to the 1.5 milestone Oct 28, 2015
@WildCryptoFox
Copy link

@flying-sheep This is a limitation of the macro muncher I'm using here. Please use --group_by or the subtle alternative of long("group-by") (inline replacement for --group-by).

@kbknapp Not a bug. Limitation in the macro. I might be able to extend the macro muncher to concat these kinds of tokens but there will need to be some distinction between --group, -by. Note that macros cannot count the characters in the identifiers it collects. This could be made strict such that -s must be before --long and any direct instances of - after --long is to be processed as the long. I may look into this with the 2.0 macro.

@WildCryptoFox
Copy link

@kbknapp I've already mentioned this issue in other issues (not tracking them down right now). And the case for -g, --group, -by would actually expand to .short("g").long("group").short("by"). The double setting of the short/long should actually lead to a panic IMHO (likely via debug_assert).

@WildCryptoFox WildCryptoFox added C-enhancement Category: Raise on the bar on expectations S-waiting-on-decision Status: Waiting on a go/no-go before implementing W: 2.x labels Oct 28, 2015
@WildCryptoFox WildCryptoFox modified the milestones: v2.0, 1.5 Oct 28, 2015
@kbknapp
Copy link
Member

kbknapp commented Oct 28, 2015

@james-darkfox yep, I remember you saying that now, my bad! I'd totally forgotten about that. We can remove the bug label, and just leave this issue open for tracking purposes.

@kbknapp kbknapp removed C-bug Category: Updating dependencies W: 1.x labels Oct 28, 2015
@WildCryptoFox
Copy link

@kbknapp :-)

@WildCryptoFox
Copy link

@kbknapp Sidenote: I could fix this for 1.0 and I might yet still do that. Feel free to jump on IRC if you'd like to discuss more about 1.0/2.0.

@flying-sheep
Copy link
Contributor Author

but there will need to be some distinction between --group, -by

i.e. “no whitespace between them”

as far as i’m concerned this is a bug since it’s very surprising, undocumented, divergent from how the usage string parser works, and subtle:

if i wouldn’t have been lucky by already having a '-b' switch defined in another argument, it would have taken ages to find this.

@WildCryptoFox
Copy link

@flying-sheep Again. Limitation of the way macros actually work. Whitespace is not a matchable token. And the solution here might be (as I said to @kbknapp, is) to make the munching work with stages such that -s must come before --long and any direct subsequent instances of - after --long to be interpreted as --long-continued.

Sorry for the lack of documentation. I wrote this macro to ease the consumption of the builder pattern. I did not get around to documenting it for clap-1.0 and instead continued to create clap-2.0 which is a complete re-write of clap focusing on getting the implementation done right without becoming huge or too complex to maintain (like clap-1.0 has become).

I will take an attempt to for 2.0 to fix this issue. After doing so, I may also back-port it to the 1.0 code. Sorry for the inconvenience.

As for being divergent from the usage string. The 2.0 is closer to the actual usage string format. https://github.com/james-darkfox/clap-rs/blob/redesign/src/macros.rs#L61 2.0 is far from being ready

@WildCryptoFox
Copy link

@flying-sheep Feel free to join irc.mozilla.org:6697+ssl #clap-rs if you'd like to discuss this in real-time.

How the rust macro system sees --group-by: - - group - by. Whitespace does not exist. The hyphens are processed purely as 'tokens' and never make their way down to the expression level where they would be processed as operators. The identifiers group and by are processed as identifiers (like variable names, but not assigned to any data). The length of the identifiers is unknown at macro level and the usage for these instances is that the identifiers are converted into &'static str strings so stringify!($group_identifier) becomes "group".

@flying-sheep
Copy link
Contributor Author

ah, makes sense. hmm, that’s problematic. so the fact that “-” isn’t a valid identifier character means that they are tokens. and e.g. - g - - group is indistinguishable from -g --group

@WildCryptoFox
Copy link

@flying-sheep Indeed. The TT muncher that my macro uses to deconstruct the usage-string influenced format looks for these kinds of patterns. Macros can be tricky at times but when you get to know and use them like I do - they aren't all that difficult to write. If you're interested in learning more about macros. I recommend the following: https://danielkeep.github.io/tlborm/ (Feel free to skip past the technical introduction if it's too difficult and return to it later if you'd like to see how the macros actually work).

@kbknapp kbknapp removed this from the v2.0 milestone Jan 23, 2016
@flying-sheep
Copy link
Contributor Author

is there any progress in 2.0?

e.g. panicking if short() is called a second time?

long("group-by") is not the worst workaround, but i’d still enjoy the real deal 😄

@flying-sheep
Copy link
Contributor Author

And apparently one can also create an app like this:

let matches = clap_app!(@app(App::new("freeform app-name ☺"))
    (author: "...")
    ...
).get_matches();

@kbknapp kbknapp changed the title allow app names, options and subcommands with hyphens allow app names, options and subcommands with hyphens when using the macro May 16, 2016
@Arnavion
Copy link
Contributor

Arnavion commented Nov 4, 2016

I just hit this too. Is it not acceptable to add case to the macro that handles --$long:expr in addition to --$long:ident? The latter would stringify $long and pass it to $arg.long as it does now, and the former would directly pass it to $arg.long (which means the expr must be a string). That way the OP's example could be written as (@arg group_by: -g --"group-by" +takes_value "foo")

Edit: Alas, Rust doesn't allow tt* after an expr...

Edit 2:

    (@arg ($arg:expr) $modes:tt --($long:expr) $($tail:tt)*) => {
        clap_app!{ @arg ($arg.long($long)) $modes $($tail)* }
    };

works. It allows (@arg group_by: -g --("group-by") +takes_value "foo") to compile as expected. Acceptable?

@kbknapp
Copy link
Member

kbknapp commented Jan 30, 2017

This is implemented as --("some-name") by Arnavion

@kbknapp kbknapp closed this as completed Jan 30, 2017
@g2p
Copy link

g2p commented Jun 11, 2018

There's a fix implemented for long arguments, sure, but subcommands (and the app name) still cannot have dashes it seems. Could we have a workaround for those too? Should I open a new issue?

@kbknapp
Copy link
Member

kbknapp commented Jun 13, 2018

@g2p yeah please open a new issue so it gets more visibility than this older one 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-enhancement Category: Raise on the bar on expectations S-waiting-on-decision Status: Waiting on a go/no-go before implementing
Projects
None yet
Development

No branches or pull requests

6 participants