Skip to content

Replace clap with structopt#648

Merged
marco-c merged 2 commits intomozilla:masterfrom
dnaka91:structopt
Jul 30, 2021
Merged

Replace clap with structopt#648
marco-c merged 2 commits intomozilla:masterfrom
dnaka91:structopt

Conversation

@dnaka91
Copy link
Contributor

@dnaka91 dnaka91 commented Jun 18, 2021

This replaces the manual setup of argument parsing through clap with structopt. It makes input handling in my opinion more simple and avoids the mismatch between argument definition and retrieval as structopt makes sure that all input parameters can be converted into the defined types.

I diffed the old help output with the new one to make sure that the differences are kept to a minimum.

While implementing the changes I potentially found a bug in regards to the path_mapping argument. It is defined as an argument that takes 1 or more values, thus allowing to give multiple values at once. Later it is parsed as a single value only though. In this new version it correctly allows only one value as input.

@marco-c
Copy link
Collaborator

marco-c commented Jul 22, 2021

The handling of the parameters is indeed simpler this way, but according to clap-rs/clap#1574 (comment) structopt is going to be deprecated in favor of clap_derive.
Maybe we should hold off on this until the situation is clearer?

@dnaka91
Copy link
Contributor Author

dnaka91 commented Jul 23, 2021

I've used structopt as well as the current clap 3.0.0-beta.2 which includes the derive feature directly in clap.
Both are almost identical because they copied most of the code from structopt. Switching forth and back between them is therefore very easy and a future upgrade to clap 3.0 should be a quick change. Most of it is just switching the keywords for the derive options.

Besides that, between claps beta 1 and 2 a long time passed by, so it'll probably still take quite a long while until the full 3.0 is released.

With all that said I would recommend using structopt now. It's unclear when clap 3.0 will be out and the upgrade from structopt to clap is very easy (which I would happily do in the future).

Copy link
Collaborator

@calixteman calixteman left a comment

Choose a reason for hiding this comment

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

I don't see any obvious errors and overall it looks better with structopt.
So r+.

@marco-c marco-c merged commit c8dccaf into mozilla:master Jul 30, 2021
@dnaka91 dnaka91 deleted the structopt branch July 30, 2021 12:14
@dnaka91
Copy link
Contributor Author

dnaka91 commented Jul 30, 2021

Thanks for the merge ❤️

I extra diffed the help output before and after to make sure the visible interface has as few changes as possible.
Now that this is merged I'll start on the cargo-grcov implementation soon.

@marco-c
Copy link
Collaborator

marco-c commented Jul 30, 2021

Thanks for the merge heart

Thank you for the contribution 😉

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