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.

Sign up for GitHub

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

Jump to bottom

Fallback for ValueEnum #5885

Open
2 tasks done
ModProg opened this issue Jan 19, 2025 · 6 comments · May be fixed by #5886
Open
2 tasks done

Fallback for ValueEnum #5885

ModProg opened this issue Jan 19, 2025 · 6 comments · May be fixed by #5886
Labels
C-enhancement Category: Raise on the bar on expectations

Comments

@ModProg
Copy link
Contributor

ModProg commented Jan 19, 2025

Please complete the following tasks

Clap Version

4.5.26

Describe your use case

I'd like to have support for a fallback option when using ValueEnum.

Describe the solution you'd like

When derive(ValueEnum) is used I'd like to have a variant e.g., Other(String), annotated with something like #[clap(other|fallback)] to contain the literal value if none of the enum variants matches.

Alternatives, if applicable

Using value_parser to and skip like so:

#[derive(Debug, Parser)]
struct Args {
    #[clap(value_parser = enum_parser)]
    arg: Enum,
}

#[derive(ValueEnum, Clone, Debug)]
enum Enum {
    A,
    B,
    #[clap(skip)]
    Other(String),
}

fn enum_parser(value: &str) -> Result<Enum, Infallible> {
    Ok(Enum::from_str(value, true).unwrap_or_else(|_| Enum::Other(value.to_owned())))
}

Additional Context

No response
@ModProg ModProg added the C-enhancement Category: Raise on the bar on expectations label Jan 19, 2025
@ModProg ModProg linked a pull request Jan 20, 2025 that will close this issue
Draft
@ModProg
Copy link
Contributor Author

ModProg commented Jan 20, 2025

I tried to implement a solution, but it turns out this is not a purely proc-macro change.

This would require to either add fallback to the ValueEnum trait or make EnumValueParser respect ValueEnum's from_str implementation.

@ModProg
Copy link
Contributor Author

ModProg commented Jan 20, 2025

Not sure what your preferred implementation would be here, should you want to implement this.

@epage
Copy link
Member

epage commented Jan 20, 2025

Serde calls such an attribute [other].

Adding to ValueEnum would be a breaking change. Any thoughts on what that would look like? Changing EnumValueParser might be able to be considered a breaking change.

An important question for this is also how to handle different types. One option is to only accept String and related types (OsString, PathBuf). Another is to use value_parser! on the variant's type and allow other = value_parser.

We'd also need to look at the impact on dynamic completions in case it affects the design at all.

@ModProg
Copy link
Contributor Author

ModProg commented Jan 20, 2025

other might be the better name then.

I'd have gone for only String based types for now, though I could see someone wanting to use, e.g., a number. Though then you might want to have multiple fallback or others, this could be expanded on in the future, if we design the change to EnumValueParser or ValueEnum correctly.

Currently, EnumValueParser is just ignoring the implementation of ValueEnum::from_str I don't know if this is intentional. Changing that is a breaking change I guess, though it is one that is quite unlikely to affect anyone, as ValueEnum::from_str will be the default implementation for most, which matches the implementation in EnumValueParser AFAICT.

Adding a new function with a default implementation to ValueEnum would also only be a breaking change for those that manually implemented parsing for generic ValueEnums which is probably also not very common.

For dynamic completions I'd have said it should be considered skipped as it is not a variant that could be completed (at least for the simple String case, if we supported value_parser then there could be completions).

To support completions for the other variant, I think we'd definitely need to change the API of ValueEnum.

@epage
Copy link
Member

epage commented Jan 20, 2025

Currently, EnumValueParser is just ignoring the implementation of ValueEnum::from_str I don't know if this is intentional. Changing that is a breaking change I guess, though it is one that is quite unlikely to affect anyone, as ValueEnum::from_str will be the default implementation for most, which matches the implementation in EnumValueParser AFAICT.

I don't remember the details but it looks like EnumValueParser offers better error messages.

@ModProg
Copy link
Contributor Author

ModProg commented Jan 20, 2025

I don't remember the details but it looks like EnumValueParser offers better error messages.

Yeah, ValueEnum returns a String error. To keep the better error messages, one approach could be to just call ValueEnum::from_str and ignore the error message:

clap/clap_builder/src/builder/value_parser.rs

Lines 1119 to 1126 in f89134d

let value = ok!(E::value_variants()
.iter()
.find(|v| {
v.to_possible_value()
.expect("ValueEnum::value_variants contains only values with a corresponding ValueEnum::to_possible_value")
.matches(value, ignore_case)
})
.ok_or_else(|| { 

let value = E::from_str(value, ignore_case).map_err(|| {

The other would be to move the generating of a proper error inside ValueEnum but that would require either adding a new function or changing from_str.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
C-enhancement Category: Raise on the bar on expectations
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

ValueEnum fallback ModProg/clap
2 participants
@epage @ModProg