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

Use gumdrop or structopt for CLI option parsing #171

Closed
emschwartz opened this issue Jul 26, 2019 · 15 comments
Closed

Use gumdrop or structopt for CLI option parsing #171

emschwartz opened this issue Jul 26, 2019 · 15 comments
Assignees
Milestone

Comments

@emschwartz
Copy link
Member

Instead of clap

Based on suggestion from @tarcieri in #169

@tarcieri
Copy link
Collaborator

tarcieri commented Jul 26, 2019

Note that structopt will get you a similar proc macro-based approach to declaring argument parsers and subcommands, although with many more dependencies. That said to me it's felt like a bit of a clunky add-on to clap, and it seems that's a known problem they intend to address in clap v3.

I wrote up some of the tradeoffs between clap/structopt and gumdrop here:

https://iqlusion.blog/introducing-abscissa-rust-application-framework#crates-we-didn39t-use-but-are-still-awesome-a_2

gumdrop/gumdrop_derive feel more cohesive to me, but the out-of-the-box UX is a bit worse, and without a recent version of Rust you can't parse arguments directly into PathBuf (the required conversion impls landed in 1.35 or 1.34 or so, don't remember offhand).

That said, if you're looking for a powerful option parser with minimal dependencies, gumdrop is my go-to pick.

@bstrie
Copy link
Contributor

bstrie commented Jul 26, 2019

I came here to mirror the above suggestion that we consider holding off until we can determine if a clap v3 release is reasonably imminent. As part of my audit of our dependencies I intend to also make a list of crates that could "use help", e.g. tower-web and clap (for whom v3 is fairly overdue due to low (but not lapsed) activity).

@bstrie
Copy link
Contributor

bstrie commented Jul 26, 2019

I also have a background goal right now of reducing compilation times in general, for which my first investigation is whether we can get away with using fewer macros (which is to say, we should be careful about adopting any new crate that provides a macro).

@tarcieri
Copy link
Collaborator

@bstrie proc-macro2 and syn are probably among the two biggest common offenders you'll see compile-time wise. However, the only way to avoid them is to purge all proc macros from your program. Personally, I think proc macros are incredibly useful and worth the hit in compilation time. Where things get really bad is if e.g. syn versions are unaligned between your dependencies and you end up compiling syn more than once.

@dora-gt
Copy link
Collaborator

dora-gt commented Aug 16, 2019

After reading issues regarding CLI arguments problem, it seems that no conclusion has been made.
Summarizing the situation, it seems to be...(if something is wrong, let me know)

  • Clap 2 (holding off)
    • - many dependencies
    • - not proc macros-first approach
  • Clap 3
    • + less dependencies
    • + proc macros-first approach
  • structopt
    • + less dependencies
    • - not proc macros-first approach
  • gumdrop
    • + less dependencies
    • + proc macros-first approach
    • - less functionalities

There seems to be Clap 3 on v3-master branch. It says Clap 3 covers structopt. So structopt might be out of choice. Then the options are Clap 3 or gumdrop. tarcieri's post says that gumdrop has less functionality. As conclusion, I felt Investigating whether Clap 3 is ready or not was the answer.

What do you think?

@emschwartz
Copy link
Member Author

Investigating whether Clap 3 is ready sounds good to me. I don't think we have very unusual needs so I'd guess if it works at all, it'll work for us

@bstrie
Copy link
Contributor

bstrie commented Aug 17, 2019

Alternatively, if we don't have unusual needs it may be that gumpdrop will suffice despite being less featureful, so it's worth at least a cursory investigation as well. That said I can believe that upgrading to Clap 3 would be less effort, so if it doesn't introduce tons of breaking changes then perhaps that will be the deciding factor (for now, anyway).

@tarcieri
Copy link
Collaborator

If you want gumdrop++, you can always use just the command-line parsing bits of Abscissa, which more or less uses gumdrop as a library but handles the user-facing interactions and hopefully provides a more clap-like user experience.

abscissa_core heavily leverages Cargo features, so you can shut off the bits you don't need.

Worst case, migrating to clapv3 should be easy since they both use similar proc macro-based argument parsing attribute "DSLs".

@dora-gt
Copy link
Collaborator

dora-gt commented Aug 21, 2019

I'll do:

  1. quick functionality investigation
  2. compilation time comparison

then we'll be able to talk about the choice.

@dora-gt
Copy link
Collaborator

dora-gt commented Aug 21, 2019

https://github.com/interledger-rs/cli-investigation

I did some investigations on Clap 3, gumdrop and structopt. I haven't checked the compilation time because Clap 3 was not actually ready... Anyway, here is a brief overview.

  • Clap 3
    • NOT READY. clap_derive is not working. We can not implement using macros right now.
    • We cannot expect a feature-complete release soon because the activities are not lively.
  • gumdrop
    • Super light but not so functional.
      • Cannot take environment variables.
      • Cannot read from files.
    • I'm not sure we should proactively adopt this one.
  • structopt
    • structopt generates codes of clap. i.e. structopt depends on clap.
    • structopt could help implement using macros.

So... let's go back to the original objective. The original objective was, AFAIK, "codes of CLI are so complex that Clippy warns it" (#169). We cannot adopt Clap 3 because it is not ready. So the options are:

  • gumdrop
  • structopt with proc macros
  • Clap 2 and separate the functions into crates respectively

hmmm... I'm not so in favor of gumdrop because we have to implement some features on our own. Maybe try the following options in order?

  1. Clap 2 and separate the functions into different crates respectively
    • ETH SE is already in this form.
  2. use structopt to use macros and make it more readable

What do you think?

@emschwartz
Copy link
Member Author

Is there any downside to using structopt? That seems fine to me (though I haven't looked into it too much)

@dora-gt
Copy link
Collaborator

dora-gt commented Aug 21, 2019

The downside would be adding these dependencies to the project:

image

but some of these are already included in the project, so it actually adds

  • structopt-derive
  • proc-macro2
  • heck
  • unicode-segmentation

It might be worth adding. OK, so if there are no objections, let's start refactoring with structopt.

@dora-gt
Copy link
Collaborator

dora-gt commented Aug 21, 2019

BTW the dependencies graph of Interledger.rs is like:
interledger-rs

haha 😂

@tarcieri
Copy link
Collaborator

proc-macro2, quote, and syn are necessary for writing proc macros in general.

It'd be nice if they started getting shipped with the Rust distribution or something.

dora-gt added a commit to dora-gt/interledger-rs that referenced this issue Aug 28, 2019
dora-gt added a commit to dora-gt/interledger-rs that referenced this issue Aug 29, 2019
Every command accepts arguments, config file settings, env vars. `interledger` and
`interledger-settlement-engines` are almost consistent.

Signed-off-by: Taiga Nakayama <dora@dora-gt.jp>

interledger#171, interledger#113, interledger#206, interledger#194, interledger#215
@dora-gt dora-gt mentioned this issue Aug 29, 2019
15 tasks
dora-gt added a commit to dora-gt/interledger-rs that referenced this issue Sep 8, 2019
Every command accepts arguments, config file settings, env vars. `interledger` and
`interledger-settlement-engines` are almost consistent.

BREAKING CHANGE: Some arguments are now obsolete. On interledger package: server_secret ->
secret_seed, btp_server -> btp_server_url, http_server -> http_server_url, redis_connection ->
redis_url, http_address -> http_bind_address, settlement_address -> settlement_api_bind_address,
btp_address -> btp_bind_address, btp_uri -> btp_server_url, http_url -> http_server_url. On
interledger-settlement-engines package: http_address -> settlement_api_bind_address,
ethereum_endpoint -> ethereum_url, redis_uri -> redis_url.

Signed-off-by: Taiga Nakayama <dora@dora-gt.jp>

interledger#171, interledger#113, interledger#206, interledger#194, interledger#215
@emschwartz
Copy link
Member Author

Closed by #290

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants