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

derive: default to parse_os_str for OsString or PathBuf types #3496

Closed
2 tasks done
joshtriplett opened this issue Feb 22, 2022 · 3 comments · Fixed by #3742
Closed
2 tasks done

derive: default to parse_os_str for OsString or PathBuf types #3496

joshtriplett opened this issue Feb 22, 2022 · 3 comments · Fixed by #3742
Labels
A-derive Area: #[derive]` macro API C-enhancement Category: Raise on the bar on expectations E-medium Call for participation: Experience needed to fix: Medium / intermediate
Milestone

Comments

@joshtriplett
Copy link
Contributor

Please complete the following tasks

Clap Version

3.1.1

Describe your use case

Most CLI applications should handle any path the user provides, not just UTF-8 paths. I'd like to make that easier to do by default when using derive, without having to remember to change the parse function.

Describe the solution you'd like

Currently, according to the documentation, parsing non-UTF-8 paths requires using parse(from_os_str).

I'd love to see this become the default if deriving an argument with a field type of PathBuf or OsString, to automatically do the right thing with paths.

Alternatives, if applicable

No response

Additional Context

No response

@joshtriplett joshtriplett added the C-enhancement Category: Raise on the bar on expectations label Feb 22, 2022
@epage epage added A-derive Area: #[derive]` macro API E-medium Call for participation: Experience needed to fix: Medium / intermediate labels Feb 22, 2022
@epage epage added this to the 3.2 milestone Feb 22, 2022
@epage
Copy link
Member

epage commented Feb 22, 2022

Huh, I know I had considered this at one point but I can't find an issue for it. I do wonder / hope for #2683 to help with this class of problem in the long term.

I know one odd thing is for us to define how we detect these types since we just get the textual name (iirc). For example, for external subcommands, we only support String and OsString, so users have to do use std::ffi::OsString (ie we don't support ffi::OsString, std::ffi::OsString, ::std::ffi::OsString, or any kind of use defined alias).

As for when to do this, an awkward thing is we haven't fully defined what our compatibility story is because users might set attributes assuming what attributes we set or make direct calls on a generated Command or ArgMatches. I'm leaning towards this being considered a "minor incompatibility" of the level of MSRV change and leaving it to a minor release, as compared to major or patch.

@joshtriplett
Copy link
Contributor Author

joshtriplett commented Feb 22, 2022

@epage One approach that might be viable on a clap 4 timeframe would be to do all conversions by way of bytes, and then conversions to String can check for UTF-8, while conversions to OsString or PathBuf or similar wouldn't. (On a comparable timeframe, we might manage to make OsString's use of bytes transparent in the standard library.) I'm also hoping such a change would make it easy to handle BString.

Meanwhile, in the short term, it seems vaguely reasonable to have the automatic behavior work the same way you're currently handling Vec.

@epage
Copy link
Member

epage commented Apr 29, 2022

Just realized, changing any of this in the short term might be considered a breaking change since we would be implicitly setting allow_invalid_utf8 which if someone directly accesses the field via ArgMatches, will now panic.

Something we've not fully defined is our expectations for the derive: how much can people expect from the behavior of the derive when they do not use it in a self-contained way (pairing the generated Args with FromArgMatches). At the moment, we do not allow people deriving them independently at least.

@epage epage modified the milestones: 3.2, 4.0 Apr 29, 2022
epage added a commit to epage/clap that referenced this issue May 23, 2022
For clap 3, its opt-in as a precaution against breaking
compatibility in some weird cases.

This does require the types to implement `Clone`.

Fixes clap-rs#3734
Fixes clap-rs#3496
Fixes clap-rs#3589
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 E-medium Call for participation: Experience needed to fix: Medium / intermediate
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants