Path & PathBuf missing FromStr trait #44431

mqudsi · 2017-09-08T19:07:53Z

Both std::path::Path and std::path::PathBuf implement std::convert::From<String> but neither one implements the std::str::FromStr trait. Since both Path and PathBuf have infallible conversions from string types, they should implement std::str::FromStr (esp. so that they can be used as generic "convertible from str" types).

Note that if something is done about rust-lang/rfcs#2143, this would be a moot discussion.

The text was updated successfully, but these errors were encountered:

I'm not quite sure how best to handle loading the license template from a path -- I mean obviously I know *how* to do it, but I'm not sure where to fit it in the codebase :) So this first attempt puts the license template directly into the config file. These are my misgivings about the license template config option as a path to a file (I'd love feedback if some of these are wrong or can be easily circumvented!): 1. I thought the obvious choice for the type of `license_template` in `create_config!` should be `PathBuf`, but `PathBuf` doesn't implement `FromStr` (yet? see rust-lang/rust#44431), so it would have to be wrapped in a tuple struct, and I went down that road for a little while but then it seemed like too much ceremony for too little gain. 2. So a plain `String` then (which, mind you, also means the same `doc_hint()`, i.e. `<string>`, not `<path>` or something like that). The fact that it's a valid path will be checked once we try to read the file. 3. But where in the code should the license template be read? The obvious choice for me would be somewhere in `Config::from_toml()`, but since `Config` is defined via the `create_config!` macro, that would mean tight coupling between the macro invocation (which defines the configuration option `license_template`) and its definition (which would rely on the existence of that option to run the template loading code). 4. `license_template` could also be made a special option which is hardwired into the macro. This gets rid of the tight coupling, but special-casing one of the config options would make the code harder to navigate. 5. The macro could maybe be rewritten to allow for config options that load additional resources from files when the config is being parsed, but that's beyond my skill level I'm afraid (and probably overengineering the problem if it's only ever going to be used for this one option). 6. Finally, the file can be loaded at some later point in time, e.g. in `format_lines()`, right before `check_license()` is called. But to face a potential *IO* error at so late a stage, when the source files have already been parsed... I don't know, it doesn't feel right. BTW I don't like that I'm actually parsing the license template as late as inside `check_license()` either, but for much the same reasons, I don't know where else to put it. If the `Config` were hand-rolled instead of a macro, I'd just define a custom `license_template` option and load and parse the template in the `Config`'s init. But the way things are, I'm a bit at a loss. However, if someone more familiar with the project would kindly provide a few hints as to how the path approach can be done in a way that is as clean as possible in the context of the codebase, I'll be more than happy to implement it! :)

I'm not quite sure how best to handle loading the license template from a path -- I mean obviously I know *how* to do it, but I'm not sure where to fit it in the codebase :) So this first attempt puts the license template directly into the config file. These are my misgivings about the license template config option as a path to a file (I'd love feedback if some of these are wrong or can be easily circumvented!): 1. I thought the obvious choice for the type of `license_template` in `create_config!` should be `PathBuf`, but `PathBuf` doesn't implement `FromStr` (yet? see rust-lang/rust#44431), so it would have to be wrapped in a tuple struct, and I went down that road for a little while but then it seemed like too much ceremony for too little gain. 2. So a plain `String` then (which, mind you, also means the same `doc_hint()`, i.e. `<string>`, not `<path>` or something like that). The fact that it's a valid path will be checked once we try to read the file. 3. But where in the code should the license template be read? The obvious choice for me would be somewhere in `Config::from_toml()`, but since `Config` is defined via the `create_config!` macro, that would mean tight coupling between the macro invocation (which defines the configuration option `license_template`) and its definition (which would rely on the existence of that option to run the template loading code). 4. `license_template` could also be made a special option which is hardwired into the macro. This gets rid of the tight coupling, but special-casing one of the config options would make the code harder to navigate. 5. Instead, the macro could maybe be rewritten to allow for config options that load additional resources from files when the config is being parsed, but that's beyond my skill level I'm afraid (and probably overengineering the problem if it's only ever going to be used for this one option). 6. Finally, the file can be loaded at some later point in time, e.g. in `format_lines()`, right before `check_license()` is called. But to face a potential *IO* error at so late a stage, when the source files have already been parsed... I don't know, it doesn't feel right. BTW I don't like that I'm actually parsing the license template as late as inside `check_license()` either, but for much the same reasons, I don't know where else to put it. If the `Config` were hand-rolled instead of a macro, I'd just define a custom `license_template` option and load and parse the template in the `Config`'s init. But the way things are, I'm a bit at a loss. However, if someone more familiar with the project would kindly provide a few hints as to how the path approach can be done in a way that is as clean as possible in the context of the codebase, I'll be more than happy to implement it! :)

kennytm · 2018-02-17T16:47:34Z

Note that, as discovered from #48292, it is impossible to implement FromStr for Path (requires unsized return value) nor &'a Path (requires GATs).

I'm not quite sure how best to handle loading the license template from a path -- I mean obviously I know *how* to do it, but I'm not sure where to fit it in the codebase :) So this first attempt puts the license template directly into the config file. These are my misgivings about the license template config option as a path to a file (I'd love feedback if some of these are wrong or can be easily circumvented!): 1. I thought the obvious choice for the type of `license_template` in `create_config!` should be `PathBuf`, but `PathBuf` doesn't implement `FromStr` (yet? see rust-lang/rust#44431), so it would have to be wrapped in a tuple struct, and I went down that road for a little while but then it seemed like too much ceremony for too little gain. 2. So a plain `String` then (which, mind you, also means the same `doc_hint()`, i.e. `<string>`, not `<path>` or something like that). The fact that it's a valid path will be checked once we try to read the file. 3. But where in the code should the license template be read? The obvious choice for me would be somewhere in `Config::from_toml()`, but since `Config` is defined via the `create_config!` macro, that would mean tight coupling between the macro invocation (which defines the configuration option `license_template`) and its definition (which would rely on the existence of that option to run the template loading code). 4. `license_template` could also be made a special option which is hardwired into the macro. This gets rid of the tight coupling, but special-casing one of the config options would make the code harder to navigate. 5. Instead, the macro could maybe be rewritten to allow for config options that load additional resources from files when the config is being parsed, but that's beyond my skill level I'm afraid (and probably overengineering the problem if it's only ever going to be used for this one option). 6. Finally, the file can be loaded at some later point in time, e.g. in `format_lines()`, right before `check_license()` is called. But to face a potential *IO* error at so late a stage, when the source files have already been parsed... I don't know, it doesn't feel right. BTW I don't like that I'm actually parsing the license template as late as inside `check_license()` either, but for much the same reasons, I don't know where else to put it. If the `Config` were hand-rolled instead of a macro, I'd just define a custom `license_template` option and load and parse the template in the `Config`'s init. But the way things are, I'm a bit at a loss. However, if someone more familiar with the project would kindly provide a few hints as to how the path approach can be done in a way that is as clean as possible in the context of the codebase, I'll be more than happy to implement it! :)

…athbuf, r=alexcrichton Implement FromStr for PathBuf Closes rust-lang#44431.

We need to hack around this issue which should be on stable soon: rust-lang/rust#44431

SimonSapin · 2018-04-22T08:09:59Z

@rust-lang/libs, do we want to revert this addition #48292 until we can add it back again with ! for the error type? #49039, #50121

alexcrichton · 2018-04-23T14:33:08Z

I'd be ok landing with a custom error type personally, still.

scottmcm · 2018-04-26T19:50:38Z

If this lands with a different custom type from string::ParseError, I suspect it wouldn't be able to be changed to ! later because of coherence. Would it be crazy to have it use string::ParseError as the we-don't-have-!-yet uninhabited error type, so that could potentially become ! later?

mqudsi · 2018-04-26T19:52:38Z

I would be for anything that lets us commit to switching it over to ! when that becomes available. Perhaps we should have a global this_will_be_bang error type...

…r=sfackler Revert "Implement FromStr for PathBuf" This reverts commit 05a9acc. The libs team was discussing rust-lang#44431 today and the changes originally added in rust-lang#48292 and the conclusion was that we'd like to revert this for now until `!` is stable. This'll provide us maximal flexibility to tweak the error type here in the future, and it looks like `!` is close-ish to stabilization so hopefully this won't be delayed for too long.

alexcrichton · 2018-05-16T17:24:02Z

Removing I-nominated as this has been dealt with and is waiting for ! stabilization

mitsuhiko · 2018-10-09T08:49:49Z

Is there a specific reason this could not use string::ParseError? This would be really nice to have and it seems like ! stabilization is quite far off.

SimonSapin · 2018-10-09T13:11:39Z

Sounds ok to me. std::string::ParseError is already stable (used in another FromStr impl) and we’ll need to deal with it somehow anyway, when we figure out the never type situation. @rust-lang/libs, any thoughts?

alexcrichton · 2018-10-09T15:28:23Z

Seems reasonable to me!

Initially landed in rust-lang#48292 and reverted in rust-lang#50401. This time, use `std::string::ParseError` as suggested in rust-lang#44431 (comment)

SimonSapin · 2018-10-17T13:56:04Z

Implemented in #55148 with FCP for insta-stable impl.

Implement FromStr for PathBuf Initially landed in rust-lang#48292 and reverted in rust-lang#50401. This time, use `std::string::ParseError` as suggested in rust-lang#44431 (comment)

Initially landed in rust-lang#48292 and reverted in rust-lang#50401. This time, use `std::string::ParseError` as suggested in rust-lang#44431 (comment)

otavio · 2018-12-21T12:22:10Z

Today I was hit by this. Surprisingly when I was looking why it did not work on stable I ended on the PathBuf from nightly and found this:

#[stable(feature = "path_from_str", since = "1.26.0")]
impl FromStr for PathBuf {
    type Err = ParseError;

    fn from_str(s: &str) -> Result<Self, Self::Err> {
        Ok(PathBuf::from(s))
    }
}

The feature gate is clearly wrong as it marks it as stable since 1.26 but it was reverted, so we should mark it accordingly, I think.

SimonSapin · 2018-12-21T12:48:11Z

Yeah I forgot to update the since attribute when unreverting in #55148. Based on the merge date it should be 1.32. Do you want to send a PR to fix it?

Path cannot implement From or FromStr directly since their methods return an owned value, but Path is dynamically-sized and so can only be used behind a pointer indirection.

Closing since I think there is nothing else to do here, but feel free to reopen or comment if this is not the case.

…eature-gate, r=Centril Fix feature gate to point to 1.32.0 for `path_from_str` When the feature has been added back (rust-lang#55148) the feature gate has not been adjusted accordingly. We have it enabled for 1.32.0, currently in Beta, so adjust it. Refs: rust-lang#44431. Signed-off-by: Otavio Salvador <otavio@ossystems.com.br>

We need to hack around this issue which should be on stable soon: rust-lang/rust#44431

TimNN added C-feature-request Category: A feature request, i.e: not implemented / a PR. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. labels Sep 17, 2017

dtolnay added C-feature-accepted Category: A feature request that has been accepted pending implementation. and removed C-feature-request Category: A feature request, i.e: not implemented / a PR. labels Nov 14, 2017

dlukes mentioned this issue Feb 16, 2018

Attempt at checking for license (#209) rust-lang/rustfmt#2456

Merged

topecongiro mentioned this issue Feb 17, 2018

Implement FromStr for PathBuf #48292

Merged

Manishearth added a commit to Manishearth/rust that referenced this issue Mar 8, 2018

Rollup merge of rust-lang#48292 - topecongiro:from_str-for-path-and-p…

f12d5aa

…athbuf, r=alexcrichton Implement FromStr for PathBuf Closes rust-lang#44431.

bors closed this as completed in #48292 Mar 9, 2018

tarcieri pushed a commit to iqlusioninc/crates that referenced this issue Apr 16, 2018

cargo-rpm: Hax for stable

70833e9

We need to hack around this issue which should be on stable soon: rust-lang/rust#44431

SimonSapin reopened this Apr 22, 2018

SimonSapin added beta-nominated Nominated for backporting to the compiler in the beta channel. S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). labels Apr 22, 2018

alexcrichton added I-nominated and removed beta-nominated Nominated for backporting to the compiler in the beta channel. labels Apr 25, 2018

alexcrichton mentioned this issue May 2, 2018

Revert "Implement FromStr for PathBuf" #50401

Merged

alexcrichton removed I-nominated S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). labels May 16, 2018

SimonSapin added a commit to SimonSapin/rust that referenced this issue Oct 17, 2018

Implement FromStr for PathBuf

a0df420

Initially landed in rust-lang#48292 and reverted in rust-lang#50401. This time, use `std::string::ParseError` as suggested in rust-lang#44431 (comment)

SimonSapin mentioned this issue Oct 17, 2018

Implement FromStr for PathBuf #55148

Merged

ischeinkman pushed a commit to ischeinkman/libnx-rs-std that referenced this issue Dec 20, 2018

Implement FromStr for PathBuf

35d9901

Initially landed in rust-lang#48292 and reverted in rust-lang#50401. This time, use `std::string::ParseError` as suggested in rust-lang#44431 (comment)

SimonSapin closed this as completed Dec 21, 2018

otavio mentioned this issue Dec 21, 2018

Fix feature gate to point to 1.32.0 for path_from_str #57040

Merged

snev68 added a commit to snev68/iqlusioninc-crates that referenced this issue Aug 5, 2024

cargo-rpm: Hax for stable

540ce48

We need to hack around this issue which should be on stable soon: rust-lang/rust#44431

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Path & PathBuf missing FromStr trait #44431

Path & PathBuf missing FromStr trait #44431

mqudsi commented Sep 8, 2017

kennytm commented Feb 17, 2018

SimonSapin commented Apr 22, 2018

alexcrichton commented Apr 23, 2018

scottmcm commented Apr 26, 2018

mqudsi commented Apr 26, 2018

alexcrichton commented May 16, 2018

mitsuhiko commented Oct 9, 2018

SimonSapin commented Oct 9, 2018

alexcrichton commented Oct 9, 2018

SimonSapin commented Oct 17, 2018

otavio commented Dec 21, 2018

SimonSapin commented Dec 21, 2018

Path & PathBuf missing FromStr trait #44431

Path & PathBuf missing FromStr trait #44431

Comments

mqudsi commented Sep 8, 2017

kennytm commented Feb 17, 2018

SimonSapin commented Apr 22, 2018

alexcrichton commented Apr 23, 2018

scottmcm commented Apr 26, 2018

mqudsi commented Apr 26, 2018

alexcrichton commented May 16, 2018

mitsuhiko commented Oct 9, 2018

SimonSapin commented Oct 9, 2018

alexcrichton commented Oct 9, 2018

SimonSapin commented Oct 17, 2018

otavio commented Dec 21, 2018

SimonSapin commented Dec 21, 2018