-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Port mdBook to clap3 #1731
Port mdBook to clap3 #1731
Conversation
mdBook's MSRV is at 1.46.0. clap is at 1.54. We are open to experimenting with lowering the MSRV (clap-rs/clap#3267) though it isn't a priority for us. Should we up mdBook's MSRV or hold off on this until either (1) clap's lowers or (2) mdBook is ready to bump? |
I think we can probably bump the MSRV. We don't have a fixed policy, but I think 8 months is probably ok. Does 3.1.6 still work on 1.54? |
Can this also update mdBook/guide/src/guide/installation.md Line 23 in 46345b8
|
Oh, and maybe add a comment in main.yml to keep them in sync (that was added in #1719, but that PR seemed to have stall). |
Updated this with the MSRV bump with the comment linking the locations. And yes, clap is still on 1.54. We had a request to extend our MSRV to a 6 month period. We are doing this on a trial basis |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
This includes resolving the deprecation warnings, one warning per commit. This was a fairly straightforward conversion, sticking to simpler parts of clap.
App::arg_from_usage
was probably the most involved. The replacement,arg!()
, doesn't seem to support kebab-case in value names.I tested by
App::debug_assert()
test--help
for each subcommandI'm assuming there is some end-to-end tests to at least catch egregious cases.
Differences I noticed (all cosmetic)
ColoredHelp
was set only on the parent before rather than withglobal_setting
, now its the default)UnifiedHelpMessage
is now the only behavior)