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

refactor: Update to clap 4 #584

Merged
merged 8 commits into from
Nov 19, 2022
Merged

refactor: Update to clap 4 #584

merged 8 commits into from
Nov 19, 2022

Conversation

berombau
Copy link
Contributor

Closes #578

  • Followed the migration guide.
  • Added features (help, usage, error-context) because of default-features = false.
  • There could be some subtle breaking changes, but the existing tests were quite extensive.
  • Removed max_term_width(90) in favor of wrap_help.
  • The default new output of --help has some subtle changes, that all seem good, reducing the total output size from 161 lines to 133 lines (with equal width COLUMNS=90).
  • The default template has changed things like color and capitalization. This is has been discussed before with bat.

TODO:

  • There are some Builder patterns like usage strings e.g. arg! that could simplify things, but these are not used here.

clap v3 hyperfine --help
image

clap v4 cargo run -- hyperfine --help
image

@sharkdp
Copy link
Owner

sharkdp commented Oct 29, 2022

Thank you very much for your contribution!

The "Min. supported rust version" check currently fails. We might need to bump the min. supported Rust version (in .github/workflows/CICD.yml and in the README).

There could be some subtle breaking changes, but the existing tests were quite extensive.

Well... hopefully not. Do you have any suspicions? Maybe parts of your changes where you're not 100% sure that the migration did the right thing? If so, we should at least check manually. Or add more integration tests, ideally.

There are some Builder patterns like usage strings e.g. arg! that could simplify things, but these are not used here.

Okay. I guess that could be done later?

src/command.rs Outdated
@@ -148,7 +154,7 @@ impl<'a> Commands<'a> {
bail!("Duplicate parameter names: {}", &duplicates.join(", "));
}
}
let command_list = command_strings.collect::<Vec<&str>>();
let command_list = command_strings;
Copy link
Owner

Choose a reason for hiding this comment

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

Hm. Do we need both? Or could we use command_list above?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I removed the unneeded command_list variables

@berombau
Copy link
Contributor Author

The MSRV of clap v4 is 1.60.0, so I bumped it to that version.
The subtle breaking changes are what the clap migration docs said, but haven't found any so far.

Copy link
Owner

@sharkdp sharkdp left a comment

Choose a reason for hiding this comment

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

Thank you!

@sharkdp sharkdp merged commit 781a681 into sharkdp:master Nov 19, 2022
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.

Update to clap v4
2 participants