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

Unable to pass a negative number as an argument #385

Closed
amandasaurus opened this issue Jan 25, 2016 · 7 comments
Closed

Unable to pass a negative number as an argument #385

amandasaurus opened this issue Jan 25, 2016 · 7 comments
Labels
A-parsing Area: Parser's logic and needs it changed somehow. C-bug Category: Updating dependencies
Milestone

Comments

@amandasaurus
Copy link

Here's a simple programme:

extern crate clap;
use clap::{App, Arg};

fn main() {
    let matches = App::new("myapp")
                    .arg(Arg::with_name("x").short("x").required(true).takes_value(true))
                  .get_matches();

    let x = matches.value_of("x").unwrap();
    println!("Value of x is {}", x);
}

It can run when you pass in 10 as an argument:

$ cargo run -- -x 10
     Running `target/debug/myapp -x 10`
Value of x is 10

However not if you pass in -10 as an argument:

$ cargo run -- -x -10
     Running `target/debug/myapp -x -10`
error: The argument '-1' isn't valid
USAGE:
    myapp [FLAGS] -x <x>

For more information try --help
An unknown error occurred

To learn more, run the command again with --verbose.

Clearly if the argument (x) takes a value, then the next string in the command line should be treated as that value, and not attempted to be parsed as a argument in it's own right.

@kbknapp
Copy link
Member

kbknapp commented Jan 25, 2016

Thanks for taking the time to file this!

Try it without using cargo run. Such as ./target/debug/myapp -- -x 10. It was actually news to me, but in #384 we found out that cargo run actually "eats" the -- and therefore clap never sees it so it's as if you're passing myapp -x 10

@amandasaurus
Copy link
Author

I ran it as "./target/debug/myapp -x -10` and got the same result.

@kbknapp
Copy link
Member

kbknapp commented Jan 25, 2016

Ah ok I misunderstood at first. This is a bug. Thanks!

@kbknapp kbknapp added C-bug Category: Updating dependencies C: args A-parsing Area: Parser's logic and needs it changed somehow. and removed T: RFC / question labels Jan 25, 2016
@kbknapp
Copy link
Member

kbknapp commented Jan 25, 2016

Also, for my curiosity did you run it as myapp -x "-10"? Because the difficulty comes in from the way clap determines if it's done parsing an options values is by looking for the leading -, this comes in to play when parsing multiple values such as -x val1 val2 val3 -y val4

The way I could fix this bug is by having something like if you set arg.num_values(1) it will parse the next value as it's value no matter what (regardless of leading hyphen). The downside is if someone makes a typo and forgets the value -x -y val2 instead of -x val1 -y val2 they will get a strange(r) error message about not expecting val2 vs the normal message of -x missing a value.

@amandasaurus
Copy link
Author

Also, for my curiosity did you run it as myapp -x "-10"?

Just tried that. Same buggy outcome. Even if that worked it would be a bug. However wrapping things in " is probably pointless, because shells like bash will strip that before calling the programme.

If I ran it with -x "\"-10\"" it parsed, but it set the value of x to "-10", not -10, so it's not the same thing at all.

@kbknapp
Copy link
Member

kbknapp commented Jan 25, 2016

Ok good to know. Also, it'll always parse values to a string, it's up to the consumer to do otherwise. So 10 will always be "10" (same with negative numbers) because parsing to numbers isn't something everyone needs or wants to spend cycles on. Plus some may one u8 some may want i64, etc.

If you want to pull a number out of a value there is a macro to remove the boiler plate called value_t! which returns a Result<T>. So something like, value_t!(m.value_of("x"), i64);. Also if you'd like to exit with an error message upon a failed parse you can just append .unwrap_or_else(|e| e.exit()); (Note, v2 soon to be release provides a better macro calling convention of value_t!(m, "x", i64); which still returns a Result<T>).

I'll play with some ideas about this bug though and post back with any updates.

@kbknapp kbknapp added this to the v2.0 milestone Jan 25, 2016
kbknapp added a commit that referenced this issue Jan 26, 2016
By using AppSettings::AllowLeadingHyphen values starting with a
leading hyphen (such as a negative number) are supported. This
setting should be used with caution as it silences certain
circumstances which would otherwise be an error (like forgetting
a value to an option argument).

Closes #385
@kbknapp
Copy link
Member

kbknapp commented Jan 26, 2016

Closed with #390 (This will be available shortly on 2.x release)

@kbknapp kbknapp closed this as completed Jan 26, 2016
kbknapp added a commit that referenced this issue Jan 28, 2016
By using AppSettings::AllowLeadingHyphen values starting with a
leading hyphen (such as a negative number) are supported. This
setting should be used with caution as it silences certain
circumstances which would otherwise be an error (like forgetting
a value to an option argument).

Closes #385
kbknapp added a commit that referenced this issue Jan 28, 2016
By using AppSettings::AllowLeadingHyphen values starting with a
leading hyphen (such as a negative number) are supported. This
setting should be used with caution as it silences certain
circumstances which would otherwise be an error (like forgetting
a value to an option argument).

Closes #385
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. C-bug Category: Updating dependencies
Projects
None yet
Development

No branches or pull requests

2 participants