Skip to content

rust: add nice command-line argument parsing#4397

Merged
wchargin merged 1 commit intomasterfrom
wchargin-rust-cli
Nov 30, 2020
Merged

rust: add nice command-line argument parsing#4397
wchargin merged 1 commit intomasterfrom
wchargin-rust-cli

Conversation

@wchargin
Copy link
Contributor

Summary:
This patch outfits //tensorboard/data/server with a nice CLI, courtesy
of clap. The flags are a subset of the main TensorBoard flags, but
with kebab-case rather than snake-case (per everyone-other-than-Google
style, via clap defaults). Since we now accept a --port argument,
which may be 0, we also print the port to which the server is bound.

The clap approach to argument parsing is “define a struct all of whose
fields implement FromStr, then mark it #[derive(Clap)] to generate
an argument parser”. This is great on both the static typing and runtime
performance fronts. To add more arguments, we just add more fields to
struct Opts. See https://docs.rs/clap/3.0.0-beta.2 for details (this
syntax is introduced in clap v3.x, which is still in beta).

Test Plan:
Run bazel run //tensorboard/data/server with -h and --help to see
short and long help text, respectively. Then play around with each
argument in turn.

wchargin-branch: rust-cli

Summary:
This patch outfits `//tensorboard/data/server` with a nice CLI, courtesy
of [`clap`]. The flags are a subset of the main TensorBoard flags, but
with kebab-case rather than snake-case (per everyone-other-than-Google
style, via `clap` defaults). Since we now accept a `--port` argument,
which may be `0`, we also print the port to which the server is bound.

The `clap` approach to argument parsing is “define a struct all of whose
fields implement `FromStr`, then mark it `#[derive(Clap)]` to generate
an argument parser”. This is great on both the static typing and runtime
performance fronts. To add more arguments, we just add more fields to
`struct Opts`. See <https://docs.rs/clap/3.0.0-beta.2> for details (this
syntax is introduced in `clap` v3.x, which is still in beta).

[`clap`]: https://crates.io/crates/clap

Test Plan:
Run `bazel run //tensorboard/data/server` with `-h` and `--help` to see
short and long help text, respectively. Then play around with each
argument in turn.

wchargin-branch: rust-cli
wchargin-source: 957fd21bf78382a279b705077d0b3917fc240e55
@wchargin wchargin added the core:rustboard //tensorboard/data/server/... label Nov 25, 2020
@google-cla google-cla bot added the cla: yes label Nov 25, 2020
@wchargin wchargin requested a review from stephanwlee November 30, 2020 17:49
Copy link
Contributor

@stephanwlee stephanwlee left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good but I would like to hear your response :)

Comment on lines +46 to +47
#[clap(long, default_value = "::0")]
host: IpAddr,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While I am glad validation is built-in to clap and typing can make sure we do not have, say, negative port number, I am wondering who is in charge of validating the IpAddr. Is it part of IpAddr#FromStr? If so, how about PathBuf?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it part of IpAddr#FromStr? If so, how about PathBuf?

Yep: in all cases, the FromStr implementation is used, so, for example,
<IpAddr as FromStr>, <PathBuf as FromStr>. (On the doc pages,
you can click [src] at right to see the implementations if you want.)

This is also why we impl FromStr for Seconds below.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or, maybe this helps: you can tell that the FromStr implementations
are responsible for validation because of the types:

fn from_str(s: &str) -> Result<Self, Self::Err>;

The docs for from_str also say this explicitly, but the signature is
enough to imply it: a method that returns a Result is generally
expected to perform validation of some kind and signal that through the
return value rather than panicking.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it, thanks.

@wchargin wchargin merged commit 5e34b53 into master Nov 30, 2020
@wchargin wchargin deleted the wchargin-rust-cli branch November 30, 2020 20:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cla: yes core:rustboard //tensorboard/data/server/...

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants