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

Migrate to clap 3.0.0 #184

Draft
wants to merge 2 commits into
base: devel
Choose a base branch
from
Draft

Conversation

robinkrahl
Copy link
Collaborator

@robinkrahl robinkrahl commented Dec 10, 2021

Current status:

  • 2021-12-10: compiles with clap 3.0.0 and should already work quite well

To do:

  • double-check extension handling
  • check version output and test case
  • update help text tests
    • remove help text tests?
  • use clap::ArgEnum
  • replace StructOpt/structopt with Parser/clap
  • re-evaluate overall argument handling with regard to the new clap API
  • consider making clap_generate optional
  • re-evaluate clap feature selection

As a preparation for the migration from structopt to clap 3.0.0, we add
tests for the help output of all commands.
This patch replaces structopt with clap v3.0.0.
@d-e-s-o
Copy link
Owner

d-e-s-o commented Dec 11, 2021

Thanks for the pull request! Will take a look this weekend.

@d-e-s-o
Copy link
Owner

d-e-s-o commented Dec 11, 2021

Looks good so far, @robinkrahl. It seems we may have to bump the minimum supported Rust version to at least 1.51 with these changes.

@robinkrahl
Copy link
Collaborator Author

robinkrahl commented Dec 11, 2021 via email

@d-e-s-o
Copy link
Owner

d-e-s-o commented Dec 11, 2021

Do you have an opinion on whether we should keep the help text tests after the migration?

It's not a particularly strongly held opinion, but I'd remove them. As you say, there may some value as a reminder, but ultimately I suspect that will only work until we are conditioned to accepting changes in those tests without questioning them much. I also believe we should be able to get similar value through peer reviews.

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.

2 participants