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

uses clap instead of custom cli arg parsing #271

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

kbknapp
Copy link

@kbknapp kbknapp commented Feb 23, 2020

This commit uses 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 (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
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; 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; 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
,
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

@rom1v
Copy link
Collaborator

rom1v commented Feb 23, 2020

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
@rom1v
Copy link
Collaborator

rom1v commented Aug 8, 2020

Hi,

Sorry for the delay.

I quickly looked at it today, but I have this issue:

$ ./gnirehtet uninstall
error: The argument 'port' wasn't found

This is weird, because "port" is not passed as argument to "uninstall" in clap.

Btw, I rebased on dev (I also replaced 24.24.24.24/8 (which is not valid) by 192.168.0.0/16 in the doc): pr271.

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),

Choose a reason for hiding this comment

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

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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants