uses clap instead of custom cli arg parsing #271
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Oh, that's great, thank you! 👍 I'll review as soon as I have time to work on gnirehtet. 😉 |
This commit uses [clap](https://github.com/clap-rs/clap) to handle command line arugments instead of a custom rolled solution. The benefit that `clap` handles all validation as well as providing many "quality of life" or UX improvements such as hidden alias, suggestions on typo, auto-generated help, etc. A previous branch of this project used [structopt](https://github.com/TeXitoi/structopt) (which internally uses `clap`). `structopt` makes heavy use of Rust's Custom Derive feature to make the code less verbose and more ergonomic. `clap` is nearing it's v3 release, which pulls `structopt` into the mainline code base. This commit does not use the v3 `structopt`-like feature-set since that release is still in alpha. This meanas the code is more verbose (yet just as performant), and could be changed to the `structopt`-like features once `clap` v3 is officially released. This commit also does not utilize some of `clap`'s other features like the ability to generate [Shell Completion scripts](https://docs.rs/clap/2.33.0/clap/struct.App.html#method.gen_completions) which could be added later if desired. This commit is *nearly* a 1:1 translation from the original home rolled, to using `clap`. I say "nearly" because `clap` adds many features on top of what the home rolled solution provided "for free" as in with no additional configuration. Those features include: * Argument validation: `--routes`, `--dns-servers`, and `--port` all now have built in validation. If someone enters an IP that isn't valid, or a port that isn't valid an error is returned with a friendly message prior to advancing into `gnirehtet` code. `--routes` also checks the CIDR for validity as well. * Auto generated help message * Suggestions on typos (this feature could be turned off to reduce the binary size, see below) * [Hidden Command aliases](https://docs.rs/clap/2.33.0/clap/struct.App.html#method.alias); The following hidden aliases have been added for commands: * `rt` which maps to `run`: I noticed `gnirehtet rt` used to be used for `gnirehtet run` * Long options for arguments (i.e. `--port` as well as `-p`) * [Hidden Argument aliases](https://docs.rs/clap/2.33.0/clap/struct.Arg.html#method.alias); the following hidden aliases have been added for arguments (which provides help for common typos): * `--dns-server` -> `--dns` -> `--dns-servers`: All of these map to the same thing * `--route` -> `--routes` * Long and Short help messages: `clap` allows one to specify a short message for arguments and commands when `-h` is used, and display a longer more comprehensive message when `--help` is used. * Multiple ways to get help: `clap` provides `-h`|`--help` for the base `gnirehtet` command, as well as all it's subcommands, i.e. `gnirehtet run --help`. There is also a `help` subcommand `gnirehtet help` which can be used alone or to display other commands, i.e. `gnirehtet help run`. This helps because people assume various forms of help, and should be able to find the help messages no matter how they originally try it. Binary Size: I see that binary size is a concern. This is the size after this commit: ``` ❯ ls -l target/release/gnirehtet .rwxrwxr-x 2.2M kevin 23 Feb 13:22 target/release/gnirehtet ❯ strip -g target/release/gnirehtet ❯ ls -l target/release/gnirehtet .rwxrwxr-x 1.1M kevin 23 Feb 13:22 target/release/gnirehtet ❯ strip -s target/release/gnirehtet ❯ ls -l target/release/gnirehtet .rwxrwxr-x 973k kevin 23 Feb 13:22 target/release/gnirehtet ``` So it does increase the binary size, but not dramatically. By further turning off [`clap`'s default cargo features](https://github.com/clap-rs/clap/#optional-dependencies--features), the size may be able to be reduced even more. However, I also am a firm believer that these slight increases are well worth the features they provide. Relates to Genymobile#243 wip: clap
kbknapp
force-pushed
the
cli-rust-redo
branch
from
c72dcd6
to
d0d4479
Compare
|
Hi, Sorry for the delay. I quickly looked at it today, but I have this issue: This is weird, because "port" is not passed as argument to "uninstall" in clap. Btw, I rebased on |
kmod-midori
reviewed
Jul 27, 2021
| serial: m.value_of("serial").map(ToOwned::to_owned), | ||
| dns_servers: m.value_of("dns-servers").map(ToOwned::to_owned), | ||
| routes: m.value_of("routes").map(ToOwned::to_owned), | ||
| port: value_t_or_exit!(m.value_of("port"), u16), |
There was a problem hiding this comment.
I suppose this should not be written in the current way, as some sub-commands does not even have port defined, even though port has a default value?
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This commit uses clap to handle
command line arugments instead of a custom rolled solution. The benefit
that
claphandles all validation as well as providing many "quality oflife" or UX improvements such as hidden alias, suggestions on typo,
auto-generated help, etc.
A previous branch of this project used
structopt (which internally uses
clap).structoptmakes heavy use of Rust's Custom Derive feature tomake the code less verbose and more ergonomic.
clapis nearing it's v3release, which pulls
structoptinto the mainline code base. Thiscommit does not use the v3
structopt-like feature-set since thatrelease is still in alpha. This meanas the code is more verbose (yet
just as performant), and could be changed to the
structopt-likefeatures once
clapv3 is officially released.This commit also does not utilize some of
clap's other features like theability to generate Shell Completion scripts
which could be added later if desired.
This commit is nearly a 1:1 translation from the original home rolled,
to using
clap. I say "nearly" becauseclapadds many features on topof what the home rolled solution provided "for free" as in with no
additional configuration. Those features include:
--routes,--dns-servers, and--portall nowhave built in validation. If someone enters an IP that isn't valid, or a
port that isn't valid an error is returned with a friendly message prior
to advancing into
gnirehtetcode.--routesalso checks the CIDR forvalidity as well.
binary size, see below)
commands:
rtwhich maps torun: I noticedgnirehtet rtused to be used forgnirehtet run--portas well as-p)for arguments (which provides help for common typos):
--dns-server->--dns->--dns-servers: All of these map to the same thing--route->--routesclapallows one to specify a shortmessage for arguments and commands when
-his used, and display alonger more comprehensive message when
--helpis used.clapprovides-h|--helpfor the basegnirehtetcommand, as well as all it's subcommands, i.e.gnirehtet run --help. There is also ahelpsubcommandgnirehtet helpwhichcan be used alone or to display other commands, i.e.
gnirehtet help run. This helps because people assume various forms of help, and shouldbe able to find the help messages no matter how they originally try it.
Binary Size
I see that binary size is a concern. This is the size after this commit:
So it does increase the binary size, but not dramatically. By further
turning off
clap's default cargofeatures,
the size may be able to be reduced even more. However, I also am a firm believer that these slight
increases are well worth the features they provide.
Relates to #243