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

Switch over to clap for arg parsing #2557

Closed
wants to merge 2 commits into from
Closed

Conversation

bjgill
Copy link
Contributor

@bjgill bjgill commented Mar 20, 2018

It saves us from having to parse std::env::args manually, though at the
cost of mandating that --all/--manifest-path come before any other
arguments. Will make adding additional clippy-specific args easier - see #2518

With this PR, the help message will now be:

$ cargo +nightly run --bin cargo-clippy -- -h
    Finished dev [unoptimized + debuginfo] target(s) in 0.0 secs
     Running `target/debug/cargo-clippy -h`
cargo-clippy 0.0.189
Checks a package to catch common mistakes and improve your Rust code.

USAGE:
    cargo clippy [OPTIONS] [CMD]...

OPTIONS:
        --all                     Run over all packages in the current workspace
    -h, --help                    Prints help information
        --manifest-path <PATH>    Path to the manifest to check
    -V, --version                 Prints version information

ARGS:
    <CMD>...    Commands to pass through to cargo rustc

Other options are the same as `cargo rustc`.

To allow or deny a lint from the command line you can use `cargo clippy --`
with:

    -W --warn OPT       Set lint warnings
    -A --allow OPT      Set lint allowed
    -D --deny OPT       Set lint denied
    -F --forbid OPT     Set lint forbidden

The feature `cargo-clippy` is automatically defined for convenience. You can use
it to allow or deny lints from the code, eg.:

    #[cfg_attr(feature = "cargo-clippy", allow(needless_lifetimes))]

I've manually tested that the four specific options get recognised correctly, and that the additional args get passed through to cargo rustc.

However, we can no longer provide flags that clippy knows nothing about directly - cargo clippy --frozen would now need to be supplied as cargo clippy -- --frozen. This seems to be a limitation of clap, though I don't know if it's going to be deal-breaking for this PR.

bjgill added 2 commits March 20, 2018 00:48
It saves us from having to parse std::env::args manually, though at the
cost of mandating that --all/--manifest-path come before any other
arguments. Will make adding additional clippy-specific args easier - see rust-lang#2518
`cargo clippy`, not `cargo-clippy`
@bjgill
Copy link
Contributor Author

bjgill commented Mar 20, 2018

Ah - travis has just found another scenario. Due to the way that clap swallows --, cargo clippy --all -- -D clippy would now need to be cargo clippy --all -- -- -D clippy.

Given the large amount of interface breakage that this PR seems to entail, I'm minded to withdraw this PR (unless there's consensus that it's worth it nonetheless). It just isn't going to be possible to switch to clap without breaking usage for a lot of people.

@oli-obk
Copy link
Contributor

oli-obk commented Mar 20, 2018

Due to the way that clap swallows --, cargo clippy --all -- -D clippy would now need to be cargo clippy --all -- -- -D clippy.

Ok that is a serious downside, mostly because it's not discoverable at all. Are you sure we can't work around that? Maybe open an issue at clap for our use case?

+25 −63

That and the simplicity of the new code makes me want this very much!

@oli-obk
Copy link
Contributor

oli-obk commented Mar 27, 2018

Any idas how to work around the issue wtih --? This would be a totally awesome feature to have

@bjgill
Copy link
Contributor Author

bjgill commented Mar 28, 2018

Got somewhat distracted, but have now raised clap-rs/clap#1236.

It looks as if there may be a simpler solution, however - getopts. It's much less fully featured than clap, but still a lot better than parsing std::env::args. Crucially, it looks as if it supports our use case out of the box.

@oli-obk
Copy link
Contributor

oli-obk commented Mar 28, 2018

Note that we are now passing the args from cargo-clippy to clippy-driver via env args: #2582

@bjgill
Copy link
Contributor Author

bjgill commented Mar 28, 2018

I've just submitted #2585, which makes this mostly pointless, given that it strips out most of the complex arg parsing.

I think I'll close this now. After/if #2585 gets in, I'll raise a new PR to move us over to getopts. In the longer term, we can then consider switching over to clap once/if it supports our use case.

@bjgill bjgill closed this Mar 28, 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.

2 participants