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

Bump clap to version 4 #679

Merged
merged 7 commits into from May 16, 2023
Merged

Bump clap to version 4 #679

merged 7 commits into from May 16, 2023

Conversation

sjackman
Copy link
Contributor

@sjackman sjackman commented Apr 27, 2023

Bump clap to version 4.
Bump MSRV to 1.64 for clap.

  • Use value_parser for numeric value types
  • Change possible_values to value_parser
  • Change hidden(true) to hide(true)
  • Change value_of and value_of_t_or_exit to get_one
  • Change is_present to get_flag or contains_id
  • Remove takes_value(true), since num_args(1) is the default
  • Use num_args(0) where needed

See https://github.com/clap-rs/clap/releases/tag/v3.2.0

Copy link
Contributor

@sunshowers sunshowers left a comment

Choose a reason for hiding this comment

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

Looks pretty good to me! The changes are mostly mechanical. Just one question.

src/lib.rs Outdated
Comment on lines 930 to 946
if let Some(color) = matches.get_one("color").map(String::as_str) {
if color != "auto" {
eprintln!("Warning: --color will be ignored when running with cargo-criterion. Use `cargo criterion --color {} -- <args>` instead.", color);
}
}
if matches.is_present("verbose") {
if matches.get_flag("verbose") {
eprintln!("Warning: --verbose will be ignored when running with cargo-criterion. Use `cargo criterion --output-format verbose -- <args>` instead.");
}
if matches.is_present("noplot") {
if matches.get_flag("noplot") {
eprintln!("Warning: --noplot will be ignored when running with cargo-criterion. Use `cargo criterion --plotting-backend disabled -- <args>` instead.");
}
if let Some(backend) = matches.value_of("plotting-backend") {
if let Some(backend) = matches.get_one::<String>("plotting-backend") {
eprintln!("Warning: --plotting-backend will be ignored when running with cargo-criterion. Use `cargo criterion --plotting-backend {} -- <args>` instead.", backend);
}
if let Some(format) = matches.value_of("output-format") {
if let Some(format) = matches.get_one::<String>("output-format") {
if format != "criterion" {
eprintln!("Warning: --output-format will be ignored when running with cargo-criterion. Use `cargo criterion --output-format {} -- <args>` instead.", format);
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, is there a reason for the different ways in which get_one is called here?

Copy link
Contributor Author

@sjackman sjackman May 15, 2023

Choose a reason for hiding this comment

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

When used in an if statement where the type needs to be comparable to a &str, it ought to be

if let Some() = matches.get_one::<String>("…") {

When used in an if statement where the type needs to be exactly a &str, it ought to be

match matches.get_one("…").map(String::as_str) {

I've fixed one case on line 930. Thanks for catching that style nit!

src/lib.rs Outdated
@@ -920,50 +927,51 @@ https://bheisler.github.io/criterion.rs/book/faq.html
.get_matches();

if self.connection.is_some() {
if let Some(color) = matches.value_of("color") {
if let Some(color) = matches.get_one("color").map(String::as_str) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@sunshowers This line was the odd one out.
I've changed

            if let Some(color) = matches.get_one("color").map(String::as_str) {

to

            if let Some(color) = matches.get_one::<String>("color") {

@lemmih lemmih merged commit 637010e into bheisler:master May 16, 2023
5 checks passed
nbarrios1337 added a commit to nbarrios1337/criterion.rs that referenced this pull request May 17, 2023
@sjackman sjackman deleted the sj/clap-4 branch May 17, 2023 22:46
@sjackman
Copy link
Contributor Author

Thank you for merging!

@sjackman
Copy link
Contributor Author

I see that the last stable release was version 0.4.0 on 2022-09-10. No rush, but might there be a stable release on the horizon that includes this change?

@sjackman
Copy link
Contributor Author

sjackman commented Jun 5, 2023

@lemmih Thank you for the releases, David!

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.

None yet

3 participants