Replace argparse with clap for arguments parsing. #145

jgraham · 2016-07-12T14:30:33Z

This appears to be more popular in the Rust community and doesn't seem to
be much worse, so it's probably a safer choice going forward.

This change is

jgraham · 2016-07-12T14:30:51Z

r? @andreastt

andreastt · 2016-07-12T23:52:14Z

Reviewed 3 of 3 files at r1.
Review status: all files reviewed at latest revision, 16 unresolved discussions.

a discussion (no related file):
Is there a way to force value notation, e.g. --webdriver-port [PORT] instead of --webdriver-port=<webdriver_port>? See from_usage in http://kbknapp.github.io/clap-rs/clap/struct.Arg.html.

a discussion (no related file):
This library seems to force colourised output in shells that don’t support colour, e.g. a dumb terminal. This is probably a bug in the library and I will file an issue.

Until it does, can we disable the colour feature? See http://kbknapp.github.io/clap-rs/clap/index.html.

Cargo.lock, line 5 [r1] (raw file):

version = "0.9.0"
dependencies = [
 "clap 2.9.2 (registry+https://github.com/rust-lang/crates.io-index)",

(Not a blocker.)

I don’t like how adding a new dependency also implicitly upgrades the versions of other dependencies in the same commit when you call cargo update. Is there a way to avoid that?

src/main.rs, line 46 [r1] (raw file):

fn app<'a, 'b>() -> App<'a, 'b> {
    App::new("geckodriver")
        .about("WebDriver implementation for Firefox.")

The old description was more accurate.

src/main.rs, line 48 [r1] (raw file):

        .about("WebDriver implementation for Firefox.")
        .version(crate_version!())
        .after_help("The source is available at https://github.com/mozilla/geckodriver

This belongs in --version.

src/main.rs, line 55 [r1] (raw file):

        .arg(Arg::with_name("webdriver_host")
             .long("host")
             .help("Host ip to use for WebDriver server (default: 127.0.0.1)")

Doesn’t have to be an IP, can be a hostname. Just “host” covers both.

Also the default is "localhost". We should probably reuse the DEFAULT_HOST from src/marionette.rs.

src/main.rs, line 56 [r1] (raw file):

             .long("host")
             .help("Host ip to use for WebDriver server (default: 127.0.0.1)")
             .takes_value(true))

Add .value_name("HOST") and similar metavar’s for other args.

src/main.rs, line 74 [r1] (raw file):

             .long("connect-existing")
             .requires("marionette-port")
             .help("Connect to an existing Firefox instance"))

s/Firefox/Gecko/

src/main.rs, line 76 [r1] (raw file):

             .help("Connect to an existing Firefox instance"))
        .arg(Arg::with_name("--no-e10s")
             .long("no_e10s")

Looks like with_name and long are confused.

src/main.rs, line 77 [r1] (raw file):

        .arg(Arg::with_name("--no-e10s")
             .long("no_e10s")
             .help("Start Firefox without e10s enabled"))

To make this more useful, can you update it to say “Start Firefox without multi-process support (e10s)”?

src/main.rs, line 82 [r1] (raw file):

             .multiple(true)
             .conflicts_with("log_level")
             .help("Set the level of verbosity. Pass once for DEBUG level logging and twice for TRACE level logging"))

Lowercase DEBUG and TRACE if we want to only accept lowercase strings. See comment below.

src/main.rs, line 87 [r1] (raw file):

             .takes_value(true)
             .possible_values(
                 &["fatal", "error", "warn", "info", "config", "debug", "trace"])

Previously you could pass these in any casing because the FromString trait of LogLevel converts the input string to lowercase before matching. I’m fine with using only lowercase, but then we should remove the lowercasing in the trait (src/marionette.rs).

src/main.rs, line 88 [r1] (raw file):

             .possible_values(
                 &["fatal", "error", "warn", "info", "config", "debug", "trace"])
             .help("Set internal Gecko log level"))

“Internal” could be omitted.

src/main.rs, line 98 [r1] (raw file):

    let matches = app().get_matches();

    let host = matches.value_of("webdriver_host").unwrap_or("127.0.0.1");

Reuse DEFAULT_HOST from src/marionette.rs.

src/main.rs, line 113 [r1] (raw file):

        Some(x) => match u16::from_str(x) {
            Ok(x) => Some(x),
            Err(_) => return Err((ExitCode::Usage, "Invalid marionette port".to_owned())),

s/marionette/Marionette/

src/main.rs, line 118 [r1] (raw file):

    };

        // overrides defaults in Gecko

Indentation mistake?

Comments from Reviewable

jgraham · 2016-07-19T12:50:23Z

Review status: 0 of 4 files reviewed at latest revision, 14 unresolved discussions.

a discussion (no related file):

Previously, andreastt (Andreas Tolfsen) wrote…

Is there a way to force value notation, e.g. --webdriver-port [PORT] instead of --webdriver-port=<webdriver_port>? See from_usage in http://kbknapp.github.io/clap-rs/clap/struct.Arg.html.

That notation would be incorrect because `[FOO]` indicates an optional `foo`. But assuming you mean `--webdriver-port PORT`

Cargo.lock, line 5 [r1] (raw file):

Previously, andreastt (Andreas Tolfsen) wrote…

(Not a blocker.)

I don’t like how adding a new dependency also implicitly upgrades the versions of other dependencies in the same commit when you call cargo update. Is there a way to avoid that?

In this case I think that happened because I ended up with incompatible versions of some dependency. It doesn't seem like a big problem.

src/main.rs, line 46 [r1] (raw file):

Previously, andreastt (Andreas Tolfsen) wrote…

The old description was more accurate.

Well technically, yes. But I don't know if it's more helpful to our users who don't necessarily care about implementation details.

src/main.rs, line 48 [r1] (raw file):

Previously, andreastt (Andreas Tolfsen) wrote…

This belongs in --version.

Well, that's very debatable. But I guess I don't care either way.

src/main.rs, line 55 [r1] (raw file):

Previously, andreastt (Andreas Tolfsen) wrote…

Doesn’t have to be an IP, can be a hostname. Just “host” covers both.

Also the default is "localhost". We should probably reuse the DEFAULT_HOST from src/marionette.rs.

No, it has to be an ip address in the current implementation due to the way `SocketAddr` works. I agree this isn't perfect.

src/main.rs, line 74 [r1] (raw file):

Previously, andreastt (Andreas Tolfsen) wrote…

s/Firefox/Gecko/

Does it work with anything that isn't Firefox?

src/main.rs, line 98 [r1] (raw file):

Previously, andreastt (Andreas Tolfsen) wrote…

Reuse DEFAULT_HOST from src/marionette.rs.

That doesn't work, since it has to be an ip address.

Comments from Reviewable

andreastt · 2016-07-20T13:09:02Z

Reviewed 4 of 4 files at r2.
Review status: all files reviewed at latest revision, 3 unresolved discussions.

a discussion (no related file):

Previously, jgraham wrote…

That notation would be incorrect because [FOO] indicates an optional foo. But assuming you mean --webdriver-port PORT

Yep.

src/main.rs, line 48 [r1] (raw file):

Previously, jgraham wrote…

Well, that's very debatable. But I guess I don't care either way.

Then lets move it back.

src/main.rs, line 55 [r1] (raw file):

Previously, jgraham wrote…

No, it has to be an ip address in the current implementation due to the way SocketAddr works. I agree this isn't perfect.

OK, then s/ip/IP/ and it’s fine.

src/main.rs, line 74 [r1] (raw file):

Previously, jgraham wrote…

Does it work with anything that isn't Firefox?

geckodriver should technically work with Fennec, which maja_zf added support for the other week, but if we want to keep the program description as “WebDriver implementation for Firefox” I’m happy to keep this as it is.

Comments from Reviewable

jgraham · 2016-07-21T10:04:15Z

Review status: all files reviewed at latest revision, 2 unresolved discussions.

src/main.rs, line 48 [r1] (raw file):

Previously, andreastt (Andreas Tolfsen) wrote…

Then lets move it back.

I have…

src/main.rs, line 74 [r1] (raw file):

Previously, andreastt (Andreas Tolfsen) wrote…

geckodriver should technically work with Fennec, which maja_zf added support for the other week, but if we want to keep the program description as “WebDriver implementation for Firefox” I’m happy to keep this as it is.

Fennec is called "Firefox" when released to users [1]

[1] https://play.google.com/store/apps/details?id=org.mozilla.firefox

Comments from Reviewable

This appears to be more popular in the Rust community and doesn't seem to be much worse, so it's probably a safer choice going forward.

jgraham · 2016-07-21T10:09:13Z

Review status: 3 of 4 files reviewed at latest revision, 2 unresolved discussions.

a discussion (no related file):

Previously, andreastt (Andreas Tolfsen) wrote…

Yep.

I think the end of my sentence was "the default output here is good enough".

Comments from Reviewable

andreastt · 2016-07-27T13:14:39Z

Reviewed 1 of 1 files at r3.
Review status: all files reviewed at latest revision, 1 unresolved discussion.

a discussion (no related file):

Previously, jgraham wrote…

I think the end of my sentence was "the default output here is good enough".

Fine, I can fix this later.

Comments from Reviewable

jgraham force-pushed the arguments_clap branch from 10804d5 to 70b5036 Compare July 19, 2016 12:50

Replace argparse with clap for arguments parsing.

9e05d53

This appears to be more popular in the Rust community and doesn't seem to be much worse, so it's probably a safer choice going forward.

jgraham force-pushed the arguments_clap branch from 70b5036 to 9e05d53 Compare July 21, 2016 10:05

andreastt merged commit 5179a6a into master Jul 27, 2016

jgraham deleted the arguments_clap branch October 5, 2016 18:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace argparse with clap for arguments parsing. #145

Replace argparse with clap for arguments parsing. #145

jgraham commented Jul 12, 2016 •

edited

jgraham commented Jul 12, 2016

andreastt commented Jul 12, 2016

jgraham commented Jul 19, 2016

andreastt commented Jul 20, 2016

jgraham commented Jul 21, 2016

jgraham commented Jul 21, 2016

andreastt commented Jul 27, 2016

Replace argparse with clap for arguments parsing. #145

Replace argparse with clap for arguments parsing. #145

Conversation

jgraham commented Jul 12, 2016 • edited

jgraham commented Jul 12, 2016

andreastt commented Jul 12, 2016

jgraham commented Jul 19, 2016

andreastt commented Jul 20, 2016

jgraham commented Jul 21, 2016

jgraham commented Jul 21, 2016

andreastt commented Jul 27, 2016

jgraham commented Jul 12, 2016 •

edited