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

Support non-Display types with default_value_t #4558

Open
2 tasks done
rdelfin opened this issue Dec 16, 2022 · 7 comments
Open
2 tasks done

Support non-Display types with default_value_t #4558

rdelfin opened this issue Dec 16, 2022 · 7 comments
Labels
A-derive Area: #[derive]` macro API C-enhancement Category: Raise on the bar on expectations S-waiting-on-design Status: Waiting on user-facing design to be resolved before implementing

Comments

@rdelfin
Copy link

rdelfin commented Dec 16, 2022

Please complete the following tasks

Clap Version

4.0.29

Describe your use case

Currently, there's no way (that I can figure out) to set a default value on a PathBuf using derive. You can see a minimal reproducible example here. The specific error is:

error[[E0277]](https://doc.rust-lang.org/stable/error-index.html#E0277): `PathBuf` doesn't implement `std::fmt::Display`
 --> src/main.rs:6:24
  |
6 |     #[arg(short, long, default_value_t = PathBuf::from("/some/default/path"))]
  |                        ^^^^^^^^^^^^^^^ `PathBuf` cannot be formatted with the default formatter; call `.display()` on it
  |
  = help: the trait `std::fmt::Display` is not implemented for `PathBuf`
  = note: call `.display()` or `.to_string_lossy()` to safely print paths, as they may contain non-Unicode data
  = note: required for `PathBuf` to implement `ToString`

Describe the solution you'd like

Ideally, either logic to allow for PathBufs to display correctly with the .display() call, or a way of suplementing default values let you write a custom display string.

Alternatives, if applicable

No response

Additional Context

No response

@rdelfin rdelfin added the C-enhancement Category: Raise on the bar on expectations label Dec 16, 2022
@epage epage changed the title Allow for default values with PathBufs Support non-Display types with default_value_t Dec 16, 2022
@epage epage added A-derive Area: #[derive]` macro API S-waiting-on-design Status: Waiting on user-facing design to be resolved before implementing labels Dec 16, 2022
@epage
Copy link
Member

epage commented Dec 16, 2022

If your default is literally PathBuf::from("/some/default/path"), then you can workaround this with default_value = "/some/default/path".

@rdelfin
Copy link
Author

rdelfin commented Dec 16, 2022

AH, thanks @epage! I assume default_value just calls from() on the value provided?
Either way, this resolves the issue for me, but sounds like there could be a use for the broader problem. Leave it to you all if you want to leave it open

@epage
Copy link
Member

epage commented Dec 17, 2022

Yeah, I've been wanting to do something about this but it appears we don't have an issue for this, so now we do!

Most likely this will require deref coercion, like value_builder! to know whether we can use Display or whether we need another way to eventually get it into a clap::builder::OsStr.

@cdaringe
Copy link

this thread was critical for me! the clap docs & guide didn't call out default_value = ..., just default_value_t = ...

thanks!

@epage
Copy link
Member

epage commented Mar 23, 2023

@cdaringe the docs mention both but they might not be the most obvious. #4090 is about trying to find ways to improve that.

@nickeb96
Copy link

Is it possible to specify a default value that doesn't get passed through a parser? I have something like:

#[derive(Parser)]
struct Args {
    #[arg(long, value_parser = thing_parser, default_value_t = THING_CONST)]
    thing: Thing,
}

fn thing_parser(s: &str) -> Result<Thing, Error> { ... }

Unfortunately this takes THING_CONST (which is already type Thing) and converts it into a string, passes that string to thing_parser to be reconstructed as a Thing, and then uses that as the default value for args.thing.

The example is a bit over-simplified, but basically I've run into situations where I can't implement fmt::Display for Thing because it's from an external library. Or it has a display implementation that can't reliably be parsed.

The workarounds I've used are not pretty. I've tried making wrapper types for Thing that allow it to be successfully turned into a string and back again, but this is suboptimal. I also have to unwrap the wrappers everywhere they're used. Another workaround is to have two argument structs, RawArgs and RealArgs. RawArgs has #[derive(Parser)] on it and only has basic String/Option<String> types. RealArgs is constructed from RawArgs and is what gets used everywhere. I feel like this defeats the purpose of using the derive interface.

@epage
Copy link
Member

epage commented Mar 29, 2023

I've run into situations where I can't implement fmt::Display for Thing because it's from an external library.

Within the builder API, the primary value add of defaults is to display them in help which requires Display.

For derive users, the value proposition is a little different because someone just might prefer their struct to have T rather than Option. However, the downsides to using an Option don't seem too high, so this is a motivation but not a strong one

With that said, one possible way of solving this is to expand on our auto-detection of Display vs a custom OsStr-based trait, we could also detect if neither is supported and bypass the regular default mechanism. I haven't fully thought this through on whether we can get away with only calling the user's default_value_t expression once or whether we would need to call it twice (builder and accessor are in separate functions)

Or it has a display implementation that can't reliably be parsed.

This is somewhat similar to the last point, the user seeing the default should probably be able to pass it in manually.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-derive Area: #[derive]` macro API C-enhancement Category: Raise on the bar on expectations S-waiting-on-design Status: Waiting on user-facing design to be resolved before implementing
Projects
None yet
Development

No branches or pull requests

4 participants