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

unexpected behavior when AllowLeadingHyphen is enabled #742

Closed
BurntSushi opened this issue Nov 12, 2016 · 4 comments
Closed

unexpected behavior when AllowLeadingHyphen is enabled #742

BurntSushi opened this issue Nov 12, 2016 · 4 comments
Labels
A-parsing Area: Parser's logic and needs it changed somehow. C-enhancement Category: Raise on the bar on expectations

Comments

@BurntSushi
Copy link
Contributor

I understand that there is a AppSettings::AllowLeadingHyphen and that it is application wide, but is there anyway to get a more targeted approach? For example, I have flag like this:

    -e, --regexp PATTERN ...   Use PATTERN to search. This option can be
                               provided multiple times, where all patterns
                               given are searched. This is also useful when
                               searching for a pattern that starts with a dash.

Users occasionally want to use a PATTERN that starts with a -. By default, clap disallows this to make failure modes better (which I think is a great choice by default). I can permit PATTERN to start with a leading - by enabling the AllowLeadingHyphen option application wide, but if I do this, I get unexpected behavior. Here's my clap app:

extern crate clap;

use clap::{App, AppSettings, Arg};

fn main() {
    let app = App::new("ripgrep")
        .setting(AppSettings::AllowLeadingHyphen)
        .arg(Arg::with_name("path")
             .multiple(true)
             .help("A file or directory to search. Directories are searched \
                    recursively."))
        .arg(Arg::with_name("regexp")
             .long("regexp").short("e")
             .takes_value(true)
             .multiple(true)
             .number_of_values(1)
             .conflicts_with("files")
             .value_name("pattern")
             .help("A regular expression pattern used for searching. \
                    Multiple patterns may be given."))
        .arg(Arg::with_name("files")
             .long("files")
             .help("Print each file that would be searched \
                    (but don't search)."));
    println!("{:?}", app.get_matches());
}

And here's an example invocation:

$ ./target/debug/clap-hyphen a b --files c
ArgMatches { args: {"path": MatchedArg { occurs: 4, vals: {1: "a", 2: "b", 3: "--files", 4: "c"} }, "files": MatchedArg { occurs: 1, vals: {} }}, subcommand: None, usage: Some("USAGE:\n    clap-hyphen [FLAGS] [OPTIONS] [--] [path]...") }

Notice that --files is correctly detected as a flag, but it is also put into the values of the path positional argument. If I remove the AppSettings::AllowLeadingHypen option, then the behavior returns to normal and --files is no longer part of path.

I'm not sure whether this is intended behavior or not, but I can think of at least two ways you might consider solving this problem if it is indeed unintended (but of course, keeping in mind that I'm unfamiliar with the implementation!):

  1. If --files is determined to be an actual flag, then it shouldn't show up in the value list of another Arg.
  2. Add an ArgSettings::AllowLeadingHypen that would let me explicitly say that only values for the -e/--regexp flag can start with a leading -. (I'm guessing there's probably a good reason why this doesn't already exist though!)

Thoughts? Have I missed something?

@BurntSushi
Copy link
Contributor Author

I wonder if there's a third option: don't permit leading hyphens in positional arguments, only in flag arguments. (Which may or may not be easier to implement, I don't know.)

@kbknapp
Copy link
Member

kbknapp commented Nov 12, 2016

Actually, now that you mention it, I do think there's a way I could easily allow targeted leading hyphens in options only. i.e. not command wide, and not in positional arguments. Let me play with an implementation real quick and see if I can get this working.

In an old implementation where this all stemmed from, it wasn't easy to determine for a single option, but this new idea may just work. Standby for updates!

BurntSushi added a commit to BurntSushi/ripgrep that referenced this issue Nov 13, 2016
There were two important reasons for the switch:

1. Performance. Docopt does poorly when the argv becomes large, which is
   a reasonable common use case for search tools. (e.g., use with xargs)
2. Better failure modes. Clap knows a lot more about how a particular
   argv might be invalid, and can therefore provide much clearer error
   messages.

While both were important, (1) made it urgent.

Note that since Clap requires at least Rust 1.11, this will in turn
increase the minimum Rust version supported by ripgrep from Rust 1.9 to
Rust 1.11. It is therefore a breaking change, so the soonest release of
ripgrep with Clap will have to be 0.3.

There is also at least one subtle breaking change in real usage.
Previous to this commit, this used to work:

    rg -e -foo

Where this would cause ripgrep to search for the string `-foo`. Clap
currently has problems supporting this use case
(see: clap-rs/clap#742),
but it can be worked around by using this instead:

    rg -e [-]foo

or even

    rg [-]foo

and this still works:

    rg -- -foo

This commit also adds Bash, Fish and PowerShell completion files to the
release, fixes a bug that prevented ripgrep from working on file
paths containing invalid UTF-8 and shows short descriptions in the
output of `-h` but longer descriptions in the output of `--help`.

Fixes #136, #189, #210, #230
@kbknapp kbknapp added A-parsing Area: Parser's logic and needs it changed somehow. D: intermediate C-enhancement Category: Raise on the bar on expectations labels Nov 14, 2016
@kbknapp
Copy link
Member

kbknapp commented Nov 14, 2016

Just an update, I'm still working this issue. It was a busy few days, so I didn't get near as far as I'd have liked. I should have a working PR in a few days.

@BurntSushi
Copy link
Contributor Author

@kbknapp No worries, it's a very small matter. Thanks for looking into it at all. :-)

BurntSushi added a commit to BurntSushi/ripgrep that referenced this issue Nov 18, 2016
There were two important reasons for the switch:

1. Performance. Docopt does poorly when the argv becomes large, which is
   a reasonable common use case for search tools. (e.g., use with xargs)
2. Better failure modes. Clap knows a lot more about how a particular
   argv might be invalid, and can therefore provide much clearer error
   messages.

While both were important, (1) made it urgent.

Note that since Clap requires at least Rust 1.11, this will in turn
increase the minimum Rust version supported by ripgrep from Rust 1.9 to
Rust 1.11. It is therefore a breaking change, so the soonest release of
ripgrep with Clap will have to be 0.3.

There is also at least one subtle breaking change in real usage.
Previous to this commit, this used to work:

    rg -e -foo

Where this would cause ripgrep to search for the string `-foo`. Clap
currently has problems supporting this use case
(see: clap-rs/clap#742),
but it can be worked around by using this instead:

    rg -e [-]foo

or even

    rg [-]foo

and this still works:

    rg -- -foo

This commit also adds Bash, Fish and PowerShell completion files to the
release, fixes a bug that prevented ripgrep from working on file
paths containing invalid UTF-8 and shows short descriptions in the
output of `-h` but longer descriptions in the output of `--help`.

Fixes #136, Fixes #189, Fixes #210, Fixes #230
@homu homu closed this as completed in c0d70fe Nov 21, 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. C-enhancement Category: Raise on the bar on expectations
Projects
None yet
Development

No branches or pull requests

2 participants