Drop from_str?

[pacak]

Currently to make a primitive parser you would typically do something like this

this uses FromStr

let foo = long("foo").argument("FOO").from_str::<usize>()

or even uglier to parse a PathBuf. map allows to use anything, typically it's From

let file = long("foo").argument_os("FOO").map(PathBuf::from)

Is it an improvement to change it into - less words, more uniform way of dealing

This uses FromStr

let foo = long("foo").argument::<usize>();

and this for PathBuf respectively? .os() bit at the end is somewhat important. Sadly rust typesystem is not expressive enough...
This uses From

let file = long("foo").argument::<PathBuf>().os()

You can fallback to keep using map in either way but will have to specify what type argument should return.

More yoda-speak though...

Derive API deals with those details internally.

@epage, thoughts?

[epage]

How often do you expect people to just want a string, especially in prototyping? Dropping from_str for those users adds more ceremony?

A part of me likes the separation of concerns with different functions, unsure how much that and me creating something similar in clap biases me towards the existing behavior.

let file = long("foo").argument::<PathBuf>().os()

I assume this would correctly map an OsStr to PathBuf and not accidentally do a str to PathBuf.

[epage]

Speaking of from_str, how much have you looked at the value parser system in clap? ValueParser itself would be less relevant since its for runtime typing but TypedValueParser has some interesting aspects.

In a nutshell, think of it as a FromOsStr trait so we can more generally build up any converter we need without a str / OsStr dichotomy.

Other nice aspects of the design:

• We provide a value_parser! to look up the appropriate TypedValueParser using deref specialization
  • The derive implementation reuses this to, if nothing else, make sure PathBuf gets the right parser (a problem in clap 3's original behavior)
  • Users can plug in their own types so we can integrate things like FileReader and FileWriter types into both the builder and derive APIs
• Instead of parsing smaller integers like u8 directly, we parse them as i64 and provide range validation errors for the user. As a side effect, this also provides a convenient way to narrow the range themselves over writing custom guards.
  • See Error text for derived numeric ranges is unclear clap-rs/clap#3199
• Using From, the function that takes in the value parser can also directly take numeric ranges, arrays of valid values, custom parser functions, etc

Besides the docs, the initial PR has some details

[pacak]

How often do you expect people to just want a string, especially in prototyping?

Not very often, I tend to jump straight to the type I want to work with, at least if it's FromStr

Dropping from_str for those users adds more ceremony?

If you want a string - it's still available, you just need to ask for it:

let string = long("foo").argument<String>("FOO");

Main motivation is to make things more uniform between String and OsString versions. Plus as a side effect it avoids allocating String and should reduce the code size a bit I think.

I assume this would correctly map an OsStr to PathBuf and not accidentally do a str to PathBuf.

If you use .os() at the end - yes, if you don't it would still work but will go though str.

Hmmm... Actually I can probably get it to do the right thing even without .os just by using TypeId, at least for PathBuf and OsString. It's not like it's going to work for some other types with currently existing FromStr instances. More magic though.

[pacak]

Other nice aspects of the design:

Cool will take a look.

[epage]

Hmmm... Actually I can probably get it to do the right thing even without .os just by using TypeId, at least for PathBuf and OsString. It's not like it's going to work for some other types with currently existing FromStr instances. More magic though.

https://crates.io/crates/castaway/0.2.1 is an interesting crate for limited forms of specialization. Unsure if its any different than just checking TypeId

[pacak]

https://crates.io/crates/castaway/0.2.1

Seems to use unsafe. Will poke in the internals but most likely won't be using it directly.

Anyway, thanks for asking all the right questions, will see how far I can push to get it both OsStr correct and uniform.

[epage]

Seems to use unsafe. Will poke in the internals but most likely won't be using it directly.

Personally, I feel like that shouldn't be an automatic reason to reject things.

[pacak]

I'm totally okay with using unsafe where appropriate - like dealing with pointers or networking magic, made a few pull requests to rust nix crate for that. I just feel that here there should be a way around it without unsafe.

[pacak]

Okay, from_str is gone, os is gone and user needs to implement something strange like this:

let file = positional::<String>("NAME").map(PathBuf::from); 

Naive version would do the right thing:

let file = positional::<PathBuf>("NAME"); 

In a nutshell, think of it as a FromOsStr
Yup, great idea. https://github.com/pacak/bpaf/blob/adjacent/src/from_os_str.rs

Users can plug in their own types so we can integrate things like

https://github.com/pacak/bpaf/blob/adjacent/examples/cat.rs - this can probably go into batteries, not sure how useful it in general.

Instead of parsing smaller integers like u8 directly, we parse them as i64 and provide range validation errors for the user.

If user wants a custom error - just grab a String and process it with parser... By default argument::<T>, positional::<T> and any::<T> will give user whatever error message FromStr implements + the original value. I think that's practical enough.

Now to go though all the documentation one more time cleaning up any mentionings of from_str...