Migration for is_present is poorly explained and seems overly complicated

[kornelski]

I'm finding it difficult and laborious to upgrade from clap v3 to v4. I've tried deprecated feature flag, but it was not enough, as behavior of flags has changed substantially, and the deprecation notice does not fully explain the change.

Problem 1: I expected the repository to have something like CHANGES.md (I've meant UPGRADING.md) or MIGRATION_GUIDE.md, but there isn't and the README doesn't mention upgrading. The best source of this info I can tell is old release notes of early v4 releases, but that's on page 4 of GitHub releases list, and I'm not sure if this information is up to date (could have changed in later versions).

Problem 2: In v4 all flag options started defaulting to taking a value:

                        .arg(Arg::new("quiet")
                            .long("quiet")
                            .short('q')

became --quiet <quiet>. Is that expected? I've had to add .num_args(0) to all my flags, but I'm not sure if this is the correct way to define a simple flag (I don't want yes/no arguments, repetition counters or anything fancy, just check presence).

Problem 2a: the .takes_value() and .min_values() functions lack #[deprecated] in v3, so they broke in v4, despite v3 compiling without warnings. It could have been a good place to warn about everything defaulting to taking a value.

Problem 3: The deprecation notice for is_present mentions there's contains_id and ArgAction, but does not really explain how to use them, and I couldn't find a definitive answer how to use the flags in v4. The examples folder covers advanced uses with subcommands and parsers, but I haven't noticed a basic example for the most basic boolean flag done correctly.

I've changed is_present to contains_id, because it seemed simpler and the deprecation notice did not explain how to read ArgAction::SetTrue once it's set. It still worked as expected v3 without warnings, but in v4 started always returning true. I've added .action(ArgAction::SetTrue) too, and contains_id still always returned true.

The documentation for contains_id mentions value_source, which has this example:

let m = Command::new("myprog")
    .arg(Arg::new("debug")
        .short('d')
        .action(ArgAction::SetTrue))
    .get_matches_from(vec![
        "myprog", "-d"
    ]);

assert_eq!(m.value_source("debug"), Some(ValueSource::CommandLine));

but I'm not sure if that's really the right way to replace is_present. It seems long and indirect way to approach the problem (I just want a boolean, not a source where it came from).

Docs for ArgAction::SetTrue use get_one::<bool>(), which at first glance sounded like a perfect logical replacement for is_present that fits with the other getters …but the problem is that it returns Option<&bool>, which is awkward to use. It requires matches.contains_id("quiet").copied().unwrap_or(false).

So I feel like I'm missing something obvious, and following the trail of contains_id and ArgAction docs has led me to weird solutions. Has this:

                        .arg(Arg::new("quiet")
                            .long("quiet")
                            .short('q');

matches.is_present("quiet")

really become:

                        .arg(Arg::new("quiet")
                            .long("quiet")
                            .short('q')
                            .num_args(0)
                            .action(ArgAction::SetTrue);

matches.get_one::<bool>("quiet").copied().unwrap_or(false)

[epage]

The quick summary:

• Flags should have ArgAction::SetTrue set
• Flags should not need num_args(0)
• Lookups can be simplified with get_flag

Problem 1: I expected the repository to have something like CHANGES.md or MIGRATION_GUIDE.md, but there isn't and the README doesn't mention upgrading.

I'm surprised you didn't see CHANGELOG.md when looking for CHANGES.md but that is where to look for both the changelog and the migration guide

The best source of this info I can tell is old release notes of early v4 releases, but that's on page 4 of GitHub releases list, and I'm not sure if this information is up to date (could have changed in later versions).

Huh, for some reason a 4.0.0 Release was not made despite the tag existing. The Release is created via an Action but the Action wasn't triggered for v4.0.0. I wish Github made it easier to debug Action problems.

became --quiet . Is that expected?

Yes, this was expected. Before whether an Arg took a value or not was conditioned on whether a short/long was defined. We simplified this down to always taking a value.

Problem 2a: the .takes_value() and .min_values() functions lack #[deprecated] in v3, so they broke in v4, despite v3 compiling without warnings. It could have been a good place to warn about everything defaulting to taking a value.

imo A deprecation should only be given if its actionable which means the alternative can be implemented. While a lot of changes from v3 to v4 were able to be handled that way (like ArgAction), num_args involved a lot of deep parser changes and I didn't dare risk backporting that to v3.

Problem 3: The deprecation notice for is_present mentions there's contains_id and ArgAction, but does not really explain how to use them, and I couldn't find a definitive answer how to use the flags in v4.

When writing that, I made the assumption that people would read the documentation for contains_id and SetTrue which explain the use cases, provide caveats, and include examples.

The examples folder covers advanced uses with subcommands and parsers, but I haven't noticed a basic example for the most basic boolean flag done correctly.

The second item listed in the examples/ directory is tutorial_builder which covers all of the basics. We also have a more formal version of this on docs.rs

Docs for ArgAction::SetTrue use get_one::(), which at first glance sounded like a perfect logical replacement for is_present that fits with the other getters …but the problem is that it returns Option<&bool>, which is awkward to use. It requires matches.contains_id("quiet").copied().unwrap_or(false).

Huh, looks like we forgot to update everything to get_flag. I'll go do that.

[kornelski]

Thank you for the clarifications. get_flag is what I was looking for. It'd be great to mention it in the deprecation notice, instead of contains_id.

I've found another migration obstacle: possible_values. Documentation search finds ValueParser::possible_values, but there's no example how to migrate from the old method, and it seems like some internal method. I hoped I could replace arg.possible_values(["foo", "bar"]) with .value_parser(ValueParser::possible_values(["foo", "bar"])) but that wasn't it.

[epage]

Thank you for the clarifications. get_flag is what I was looking for. It'd be great to mention it in the deprecation notice, instead of contains_id.

In replacing is_present, a user needs to migrate to ArgAction::SetTrue and everything else follows from that, so we point to that so people can read the documentation and understand how the new stuff works, which includes get_flag

I've found another migration obstacle: possible_values. Documentation search finds ValueParser::possible_values, but there's no example how to migrate from the old method, and it seems like some internal method. I hoped I could replace arg.possible_values(["foo", "bar"]) with .value_parser(ValueParser::possible_values(["foo", "bar"])) but that wasn't it.

The deprecation notice for Arg::possible_values suggests what to use, arg.value_parser(["one", "two", three"]). Arg::value_parser uses Into and ValueParser has a From for arrays with values that can be converted into a PossibleValue.

Your path to using ValueParser::possible_values based on the documentation isn't clear to me. The description is "Reflect on enumerated value properties" and it shows it takes only &self and returns values, ie its a getter. Searching for "possible_values" would have also highlighted the PossibleValuesParser which is what the earlier From/Into creates and would have been another route to what you were trying to do.

[kornelski]

ValueParser::possible_values is the first thing that pops on docs.rs when I search for possible_values. There is ValueParser::string that actually works, so it seemed plausible that ValueParser has methods for other kinds of parsers too.

.value_parser(["foo", "bar"]) is neat, but it's unsearchable and clever. I did not expect an array of options to be a parser for itself. Other argument types required a macro or parser constructor, so I went searching for a possible_values parser.