ref(core): Refactor core:fs error mapping to use unified format #17719
Conversation
|
Related: #17526 (comment) |
runtime/ops/fs.rs
Outdated
| })?; | ||
| let std_file = open_options | ||
| .open(&path) | ||
| .map_err(create_err_mapper(format!("open '{}'", path.display())))?; |
There was a problem hiding this comment.
This will always allocate a string for error even if error is not returned. Could you instead change these to use closure instead so they are lazily evaluated?
There was a problem hiding this comment.
On the one hand, this looks ugly as hell:
.map_err(|err| create_err_mapper(format!("open '{}'", path.display()))(err))?;But this one isn't any better as well :(
fn create_err_mapper(desc_fn: impl Fn() -> String) -> impl Fn(Error) -> Error {
move |err: Error| Error::new(err.kind(), format!("{err}, {}", desc_fn()))
}
// ...
.map_err(create_err_mapper(|| format!("open '{}'", path.display())))?;Any preferences?
There was a problem hiding this comment.
The first one would be fairly pretty if it were not for the create_err_mapper usage which here seems a bit redundant as it's being created inline in a lambda anyway.
Would it be possible to write a secondary err_mapper function for use in these "inline" cases when there is no real benefit in creating an error mapper once and using it in many places?
.map_err(|err| default_err_mapper(format!("open '{}'", path.display()), err))
|
Done, this should do the work. |
|
Nice work, thank you very much! |
Follow-up to #17710
I double-checked the format and name, and there should be no errors during refactoring. Trimmed down the error mapping code by 1/3rd.