Improved deprecation warnings for derive-based args

[dbrgn]

When compiling tealdeer CLI args, I got the following deprecation warning:

warning: use of deprecated variant `clap::AppSettings::ArgRequiredElseHelp`: Replaced with `Command::arg_required_else_help` and `Command::is_arg_required_else_help_set`
  --> src/cli.rs:13:31
   |
13 | #[clap(setting = AppSettings::ArgRequiredElseHelp)]
   |                               ^^^^^^^^^^^^^^^^^^^
   |
   = note: `#[warn(deprecated)]` on by default

I'm using the derive based config:

#[derive(Parser, Debug)]
#[clap(about = "A fast TLDR client", author, version)]
#[clap(setting = AppSettings::ArgRequiredElseHelp)]
#[clap(group = ArgGroup::new("command_or_file").args(&["command", "render"]))]
pub(crate) struct Args {
    /// The command to show (e.g. `tar` or `git log`)
    #[clap(min_values = 1)]
    pub command: Vec<String>,

    // ...

With this approach, the deprecation warning left me a bit unsure on how to apply this change. What I did:

• I tried simply adding arg_required_else_help to the command argument:

  #[clap(min_values = 1, arg_required_else_help = true)]
  pub command: Vec<String>,
  

  ...and also:

  #[clap(min_values = 1, arg_required_else_help(true))]
  pub command: Vec<String>,
  

  ...however, both attempts fail to compile because command is an Arg and not a Command.

• I searched the docs and looked at the method docs (including all examples), but that didn't really help.

• Finally, I looked at the derive docs and found the section about raw attributes that made me realize that I could use #[clap(arg_required_else_help(true))] directly on the main struct.

All in all, it took me maybe 10-15 minutes until I got it to work. However, it was not easy nor intuitive to find. My Args struct is not a Command, so it didn't really occur to me that I could apply command methods to it. In the end, the docs are there (when you look for them), and I got it to work, but maybe my experience can help in improving these deprecation warnings? I suspect that a big part of clap users will be using derive based args (in my opinion that's one of the most awesome parts of clap), so maybe the deprecation warnings could be improved to help those users?

[epage]

..however, both attempts fail to compile because command is an Arg and not a Command.
...
My Args struct is not a Command, so it didn't really occur to me that I could apply command methods to it.

Is the part where things went wrong the fact that we reference a struct named Command and you had a field named command and equated the two, leading you to put the attribute on the field rather than the struct?

I suspect that a big part of clap users will be using derive based args (in my opinion that's one of the most awesome parts of clap), so maybe the deprecation warnings could be improved to help those users?

The challenge is supporting all users without making the message so long no one reads it.

In the end, the docs are there (when you look for them), and I got it to work, but maybe my experience can help in improving these deprecation warnings? I

What we've been trying to teach in the documentation is the relationship between the derive and builder APIs. It isn't sustainable to ignore the builder API and document everything only for the derive, so we rely on teaching people to be able to translate concepts. We cover this in several ways in the derive reference though I can see some spots for improving it in the derive tutorial. We have #3189 for improving the documentation.

[dbrgn]

Thanks for the quick reply!

Is the part where things went wrong the fact that we reference a struct named Command and you had a field named command and equated the two, leading you to put the attribute on the field rather than the struct?

Not really, but maybe the name did influence my thinking (although I was aware of the name conflict when I looked for a solution). I mistakenly thought of the Command struct as something like a git subcommand (i.e. log in the case of git log). And since all other parameters of my program arguments are flags, command was the only one that resembled a subcommand, so I put it there.

It seems that in 3.1.0 App was renamed to Command, and that's something that I missed. If I would have been aware of that, or if the error message still used the App terminology, I might have ended up with the correct approach sooner.

The challenge is supporting all users without making the message so long no one reads it.
...
What we've been trying to teach in the documentation is the relationship between the derive and builder APIs.

Yep, I understand that. However, from a maintainer point of view: It's been a few months since I last actively read into the clap concepts (and some of those changed or were renamed in the meantime). I would have no qualms to read a longer deprecation warning if it helps me to understand the change. Maybe something like this would help?

warning: use of deprecated variant clap::AppSettings::ArgRequiredElseHelp: Replaced with Command::arg_required_else_help and Command::is_arg_required_else_help_set. Note: If you're using the derive feature, you can apply Command methods directly on the argument struct like this: #[clap(arg_required_else_help(true))].

(Note: Docs are hard. I'm not trying to criticize clap, I'm just sharing my experience because I feel that "outside views" are valuable as a maintainer. Feel free to ignore this suggestion if you think it wouldn't be helpful!)

[epage]

dbrgn:

It seems that in 3.1.0 App was renamed to Command, and that's something that I missed. If I would have been aware of that, or if the error message still used the App terminology, I might have ended up with the correct approach sooner.

However, from a maintainer point of view: It's been a few months since I last actively read into the clap concepts (and some of those changed or were renamed in the meantime).

I can see that being easy to miss and it making the process harder. If you had missed that, you probably also missed:

Proactively check for bad Command configurations by calling Command::debug_assert in a test (example)

While this won't help in all cases, it can help catch subtle behavior changes on breaking releases earlier than testing by hand.

dbrgn:

(Note: Docs are hard. I'm not trying to criticize clap, I'm just sharing my experience because I feel that "outside views" are valuable as a maintainer. Feel free to ignore this suggestion if you think it wouldn't be helpful!)

Yes and I very much appreciate these user reports. The transition for derive users was a lot harder than I had expected, for example

• Deprecating `AppSettings` caused warnings in Derive API #3488
• AppSettings is deprecated, how to update old code? #3538

I had hoped pinning #3538 would help. The problem its hard to judge effectiveness of that since you don't get reports from the people who saw that earlier in the process but from those who saw it after it took too long or those who missed it completely.