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

Use clap::ArgEnum to auto-generate possible cli arguments from Rust enums #10369

Closed

Conversation

petr-tik
Copy link

@petr-tik petr-tik commented Feb 7, 2022

What does this PR try to resolve?

I was familiarising myself with the codebase after the upgrade to clap-3.0 PR #10265 and
spotted a couple of low-hanging refactors. Vectors of strings were duplicated as cli representations
of enums. Those strings can be exhaustively auto-generated from the enum variants.

Tangentially related to #6104, since this automates the need to manually keep the
vectors of static strings up to date with the possible values of ConfigFormat and tree::Prefix.

How should we test and review this PR?

The PR is split into 2 commits - one to set up the whole cargo workspace with
clap + derive feature and refactor ConfigFormat. The second follows up with a
refactor to use clap derive ArgEnum for tree::Prefix.
Feel free to suggest tests, if you feel this is too untested as is.

Additional information

  • Confirm that the derive feature of clap is acceptable as a dependency addition

@rust-highfive
Copy link

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @ehuss (or someone else) soon.

Please see the contribution instructions for more information.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Feb 7, 2022
@Eh2406
Copy link
Contributor

Eh2406 commented Feb 7, 2022

cc @epage

@@ -34,7 +34,7 @@ pub fn exec(config: &mut Config, args: &ArgMatches) -> CliResult {
Some(("get", args)) => {
let opts = cargo_config::GetOptions {
key: args.value_of("key"),
format: args.value_of("format").unwrap().parse()?,
format: args.value_of_t_or_exit("format"),
Copy link
Contributor

Choose a reason for hiding this comment

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

We shouldn't be short-circuiting cargo's exiting. We'll change expected exit codes, if nothing else.

Copy link
Author

Choose a reason for hiding this comment

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

Thanks for the swift review. can I use value_of_t("format")? instead?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes

Copy link
Author

Choose a reason for hiding this comment

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

ta! will change to that

} else {
args.value_of("prefix").unwrap()
args.value_of_t_or_exit("prefix")
Copy link
Contributor

Choose a reason for hiding this comment

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

ditto

Comment on lines 28 to 36
impl AsRef<str> for ConfigFormat {
fn as_ref(&self) -> &str {
match self {
ConfigFormat::Toml => "toml",
ConfigFormat::Json => "json",
ConfigFormat::JsonValue => "json-value",
}
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you can use PossibleValue to help with this, like we do Display in clap_complete::Shell.

impl Display for Shell {
    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
        self.to_possible_value()
            .expect("no values are skipped")
            .get_name()
            .fmt(f)
    }
}

Copy link
Author

Choose a reason for hiding this comment

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

for my education, what's the difference between using a PossibleValue vs creating a match arm that returns static strings? Are both of them exhaustive checks?

Copy link
Contributor

Choose a reason for hiding this comment

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

This single-sources the values so you don't have to worry about typos.

Copy link
Author

Choose a reason for hiding this comment

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

To confirm, should I replace the derived clap::ArgEnum implementation with my own impl that has to_possible_value() and value_variants() implementations?

error[E0119]: conflicting implementations of trait `clap::ArgEnum` for type `ops::cargo_config::ConfigFormat`
  --> src/cargo/ops/cargo_config.rs:13:10
   |
13 | #[derive(clap::ArgEnum, Clone)]
   |          ^^^^^^^^^^^^^ conflicting implementation for `ops::cargo_config::ConfigFormat`
...
28 | impl ArgEnum for ConfigFormat {
   | ----------------------------- first implementation here

Copy link
Contributor

Choose a reason for hiding this comment

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

No, you can keep deriving, just use the functions from the trait to get the generated values. Shell doesn't derive to avoid foisting the derive feature on people not using it.

Copy link
Author

Choose a reason for hiding this comment

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

thanks for the tip and sorry if I am being daft here.

        self.to_possible_value()
            .expect("no values are skipped")
            .get_name()

this looks like a runtime check that might throw at runtime, while the current implementation is an exhaustive compile-time check. Should I still use the to_possible_value().expect("Should be fine") and if so, what's the safest way to do that?

Copy link
Contributor

@epage epage Feb 8, 2022

Choose a reason for hiding this comment

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

to_possible_value only "fails" if you have a #[clap(skip)] field. None of them do which is why an expect is ok here to act as an assert (the expect is documenting the invariant that it is relying on).

This is better because the other approach would have required tests for each field to make sure there were no typos. People adding new fields would need to remember to update those tests.

Comment on lines 117 to 124
impl AsRef<str> for Prefix {
fn as_ref(&self) -> &str {
match self {
Prefix::None => "none",
Prefix::Indent => "indent",
Prefix::Depth => "depth",
}
}
}

Copy link
Contributor

Choose a reason for hiding this comment

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

ditto

src/cargo/ops/cargo_config.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@epage epage left a comment

Choose a reason for hiding this comment

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

Looks good!

@bors
Copy link
Collaborator

bors commented Feb 17, 2022

☔ The latest upstream changes (presumably #10396) made this pull request unmergeable. Please resolve the merge conflicts.

Use the derive feature of clap to stop relying on hardcoded
vector of strings and programmatically derive config values
from the enum type
Stop returning static &str and use the clap-required
possible_values trait method
early bailing and messing up return code
delegate &str creation to the to_possible_value()
method provided by clap::ArgEnum. Since both arguments
are non-skipped, we can safely use expect()
@petr-tik
Copy link
Author

@epage @ehuss Rebased and dealt with the merge conflict reported by bors.

Can you please review if you get the chance.

Copy link
Contributor

@epage epage left a comment

Choose a reason for hiding this comment

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

Thanks for doing this!

@ehuss
Copy link
Contributor

ehuss commented Feb 20, 2022

This adds several new dependencies, and adds some more boilerplate code. Is it possible to do this without the derive macros? Are the AsRef impls necessary? Is there some way this can be trimmed down a little?

@petr-tik
Copy link
Author

Thanks for reviewing!

Is there some way this can be trimmed down a little?

Would be happy to, what specifically do you want me to trim down?

Is it possible to do this without the derive macros?

From the documentation for ArgEnum, using the trait requires the derive feature
https://docs.rs/clap/latest/clap/trait.ArgEnum.html

The AsRef implementations are needed for the value_variants trait method.

I appreciate this might increase compile-times. It brings the benefits of automatically maintaining and generating lists of possible ConfigFormats and Prefix types by delegating to clap-based derives instead of keeping those updated manually.

@epage
Copy link
Contributor

epage commented Feb 21, 2022

Is it possible to do this without the derive macros?

Since cargo is using the builder API, the main value ArgEnum is providing is automatically generating the to/from string conversions. If cargo were using the derive API, then we wouldn't need the AsRef, Display, and FromStr boiler plate. We'd just have

/// Display format
#[long, arg_enum, default_value_t = cargo_config::ConfigFormat::Toml]
format: cargo_config::ConfigFormat

Are the AsRef impls necessary? Is there some way this can be trimmed down a little?

Clap accepts &str and stores them, so the easiest strings to work with are &'static str (we plan to fix this in clap 4), so we can't use the existing Display which is why we have the AsRef for using cargo_config::ConfigFormat to set the default value rather than hand-writing the default as a string.

ConfigFormat::value_variants()
.iter()
.filter_map(ArgEnum::to_possible_value)
}
}

impl FromStr for ConfigFormat {
Copy link
Contributor

Choose a reason for hiding this comment

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

If we move forward with this, we should switch the FromStrs to be implemented in terms of ArgEnum as well

For example, clap_completes Shell:

impl FromStr for Shell {
    type Err = String;

    fn from_str(s: &str) -> Result<Self, Self::Err> {
        for variant in Self::value_variants() {
            if variant.to_possible_value().expect("No value is skipped").matches(s, false) {
                return Ok(*variant);
            }
        }
        Err(format!("Invalid variant: {}", s))
    }
}

@ehuss
Copy link
Contributor

ehuss commented Mar 23, 2022

Closing as I would prefer to avoid pulling in several new dependencies for a relatively minor improvement (and it also seems to add quite a bit more lines of code and subjectively seems more complex). Thank you for the PR, though!

@ehuss ehuss closed this Mar 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants