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

fix(derive): Provide derive-friendly deprecation messages #3832

Merged
merged 5 commits into from Jun 14, 2022

Conversation

epage
Copy link
Member

@epage epage commented Jun 14, 2022

This is a step towards #3822. I'd say this fixes it but I'd want some
user acceptance before doing so.

To do this, we

  • Mask all ArgMatches deprecation messages with assumption being that any message from these has its roots in an augment_args deprecation. You can see the raw deprecations with clap_derive/raw-deprecation
  • Create call generated functions with a targeted deprecation message that we call for each parse case.
  • Mask all deprecation messages for implicit Arg methods, delegating to the messages for the generated functions

We intentionally do not provide a deprecation message for implicit parse attributes since resolving that is just busywork that most likely won't help users in migrating to clap 4.

I had originally assumed this would be difficult to be precise in our allow and deprecations but I found place to split up the calls. Its not completely precise but it should be good enough in the short term.

Example messages

warning: use of deprecated function `<legacy::custom_string_parsers::DefaultedOpt as clap::Args>::augment_args::parse_from_str`: Replaced with `#[clap(value_parser = ...)]`
   --> tests/derive/legacy/custom_string_parsers.rs:167:25
    |
167 |     #[clap(short, parse(from_str))]
    |                         ^^^^^^^^

warning: use of deprecated function `<legacy::custom_string_parsers::DefaultedOpt as clap::Args>::augment_args_for_update::parse_try_from_str`: Replaced with `#[clap(value_parser = ...)]`
   --> tests/derive/legacy/custom_string_parsers.rs:170:25
    |
170 |     #[clap(short, parse(try_from_str))]
    |                         ^^^^^^^^^^^^


warning: use of deprecated function `<legacy::custom_string_parsers::PathOpt as clap::Args>::augment_args::parse_from_os_str`: Replaced with `#[clap(value_parser)]` for `PathBuf` or `#[clap(value_parser = ...)]` with a custom `TypedValueParser`
  --> tests/derive/legacy/custom_string_parsers.rs:23:31
   |
23 |     #[clap(short, long, parse(from_os_str))]
   |                               ^^^^^^^^^^^
   |
   = note: `#[warn(deprecated)]` on by default

warning: use of deprecated function `<legacy::custom_string_parsers::PathOpt as clap::Args>::augment_args::parse_from_os_str`: Replaced with `#[clap(value_parser)]` for `PathBuf` or `#[clap(value_parser = ...)]` with a custom `TypedValueParser`
  --> tests/derive/legacy/custom_string_parsers.rs:23:31
   |
23 |     #[clap(short, long, parse(from_os_str))]
   |                               ^^^^^^^^^^^
   |
   = note: `#[warn(deprecated)]` on by default

warning: use of deprecated function `<legacy::custom_string_parsers::Occurrences as clap::Args>::augment_args::parse_from_occurrences`: Replaced with `#[clap(action = ArgAction::Count)]` with a field type of `u8`
   --> tests/derive/legacy/custom_string_parsers.rs:207:31
    |
207 |     #[clap(short, long, parse(from_occurrences))]
    |                               ^^^^^^^^^^^^^^^^

warning: use of deprecated function `<legacy::custom_string_parsers::Occurrences as clap::Args>::augment_args::parse_from_occurrences`: Replaced with `#[clap(action = ArgAction::Count)]` with a field type of `u8`
   --> tests/derive/legacy/custom_string_parsers.rs:219:50
    |
219 |     #[clap(short, long, parse(from_occurrences = foo))]
    |                                                  ^^^

warning: use of deprecated function `<legacy::flags::non_bool_type_flag::Opt as clap::Args>::augment_args_for_update::parse_from_flag`: Replaced with `#[clap(action = ArgAction::SetTrue)]`
  --> tests/derive/legacy/flags.rs:85:47
   |
85 |         #[clap(short, long, parse(from_flag = parse_from_flag))]
   |                                               ^^^^^^^^^^^^^^^

  • We encourage busywork for users accepting a PathBuf and correctly set the parse(from_os_str). Correctly detecting this to not show it would add another level of work and we'd need to have some way to tell users when they upgrade to clap 4 what the correct replacement for parse(from_os_str) is (nothing).
  • We're not providing much guidance when users that only specify parse(from_str),. etc

epage added 5 commits Jun 14, 2022
The main thing that might slip through is `bool`.  We'll see if we an
re-introduce that.
This is a step towards clap-rs#3822.  I'd say this fixes it but I'd want some
user acceptance before doing so.
@epage epage merged commit ffd24af into clap-rs:master Jun 14, 2022
21 checks passed
@epage epage deleted the derive branch Jun 14, 2022
epage referenced this issue in stencila/stencila Jun 15, 2022
These should be able to be removed when upgrading to `clap` 4.0.
This was done rather than adding numerous `#[clap(value_parser)]` attributes.
See clap-rs/clap#3822.
epage referenced this issue in Byron/gitoxide Jun 15, 2022
I'd rather not have to deal with the new API because there is no problem
with the old one for me.

Once 4.0 becomes the default, the work has to be put in to probably
achieve the same, but I hope it's not more wordy than before.
I also hope that inconveniences will be figured out by the folks in the
mean time.

Related to clap-rs/clap#3822
epage referenced this issue in sebastinas/drt-tools Jun 15, 2022
clap >= 3.2 adds no value, so let them figure out what they want first.
This sounds like change for the sake of change.

See also clap-rs/clap#3822
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

1 participant