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

Upgrade to clap 4 #263

Merged
merged 2 commits into from
Oct 12, 2022
Merged

Upgrade to clap 4 #263

merged 2 commits into from
Oct 12, 2022

Conversation

smoelius
Copy link
Member

@smoelius smoelius commented Oct 4, 2022

No description provided.

@smoelius smoelius force-pushed the clap-4 branch 5 times, most recently from 36c9679 to b262298 Compare October 5, 2022 08:00
@@ -17,7 +17,7 @@ tempfile = "3.3"
xdg = "2.4"

[dependencies]
clap = { version = "3.2", features = ["cargo"] }
clap = { version = "4.0", features = ["cargo"] }

Choose a reason for hiding this comment

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

Tiny nit, but it might be worth considering bumping to the latest minor here (4.0.9), since they had a couple of bug fixes since 4.0.

Copy link
Member Author

Choose a reason for hiding this comment

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

I have very mixed feelings about this. But the way I have been leaning lately is to not include patch versions in Cargo.toml files. Tracking patch versions tends to introduce a lot of noise, and I prefer changes to the Cargo.toml file to be "significant," e.g., addition/removal of a dependency.

Choose a reason for hiding this comment

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

Yeah, this was based on a misunderstanding on my part. This is good to ignore.

.subcommand_required(true)
.arg_required_else_help(true)
.allow_external_subcommands(true)
.external_subcommand_value_parser(value_parser!(OsString))

Choose a reason for hiding this comment

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

Nit: I think this is the default value parser for external subcommands, so you can probably omit this line.

Copy link
Member Author

Choose a reason for hiding this comment

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

My experiments support this.

However, I think I would prefer keep this line. If someone later reviews this change, I would like it to be clear that this line was not lost:

.setting(AllowInvalidUtf8ForExternalSubcommands)

Choose a reason for hiding this comment

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

Makes sense!

.allow_hyphen_values(true)
.disable_help_subcommand(true)
.disable_help_flag(true)
.disable_version_flag(true)

Choose a reason for hiding this comment

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

Needs to be tested, but I believe subcommands now have their version flags disabled by default. So you might no longer need this line.

Choose a reason for hiding this comment

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

(Ditto for all other subcommands.)

Copy link
Member Author

Choose a reason for hiding this comment

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

I added several tests. Please tell me if you think there are still gaps, or if you do not like their approach(es).

Choose a reason for hiding this comment

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

Taking a look now.

Copy link

@woodruffw woodruffw left a comment

Choose a reason for hiding this comment

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

LGTM! Those tests are great!

@smoelius
Copy link
Member Author

Thanks a million, @woodruffw.

@smoelius smoelius merged commit 721bd4d into rust-fuzz:master Oct 12, 2022
@smoelius smoelius deleted the clap-4 branch October 12, 2022 14:23
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