-
Notifications
You must be signed in to change notification settings - Fork 193
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
rust/treefile: Include filename in more error msgs #1735
Conversation
Slightly prep for next patch (was in the area so just did the whole module).
This uses the `Context` feature of the failure crate to make error messages more useful when we fail to open a file. The difference with `map_err` is that one can still obtain the underlying error from the context if need be. Though surprisingly, the normal `Display` for a `Context` doesn't include the original error, so we essentially have to do a prefix here (see [1]). Before: ``` error: Failed to load YAML treefile: No such file or directory (os error 2) ``` After: ``` error: Failed to load YAML treefile: Can't open file "treecompose-post.sh": No such file or directory (os error 2) ``` [1] rust-lang-deprecated/failure#182
Err(ref e) if e.kind() == io::ErrorKind::InvalidInput => {} | ||
Err(ref e) => panic!("Expected invalid treefile, not {}", e.to_string()), | ||
Err(ref e) => { | ||
match e.downcast_ref::<io::Error>() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting. I need to learn more about the TypeId
stuff, I guess it boils down to a unique id for a vtable or so?
/// Open file and provide context containing filename on failures. | ||
fn open_file<P: AsRef<Path>>(filename: P) -> Fallible<fs::File> { | ||
return Ok(fs::File::open(filename.as_ref()).with_context( | ||
|e| format!("Can't open file {:?}: {}", filename.as_ref().display(), e))?); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@lucab was asking why we're doing Ok(...?)
here instead of just ...
. I think what's happening there is that we're relying on the .into()
behaviour of ?
in the case where opening the file fails. Doing just:
return fs::File::open(filename.as_ref()).with_context(
|e| format!("Can't open file {:?}: {}", filename.as_ref().display(), e));
fails with:
error[E0308]: mismatched types
--> src/treefile.rs:145:12
|
145 | return fs::File::open(filename.as_ref()).with_context(
| ____________^
146 | | |e| format!("Can't open file {:?}: {}", filename.as_ref().display(), e));
| |________________________________________________________________________________^ expected struct `failure::Error`, found struct `failure::Context`
|
= note: expected type `std::result::Result<_, failure::Error>`
found type `std::result::Result<_, failure::Context<std::string::String>>`
Similarly, doing .into()
at the end will also fail because there's no conversion implementation from Result<T, E>
to Result<T, failure::Error>
. Whereas failure::Context<T>
can be converted to failure::Error
via the blanket Fail
implementation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel like there's probably a .map_err()
way to do this that would be more elegant but not worth redoing!
⚡ Test exempted: merge already tested. |
This uses the `Context` feature of the failure crate to make error messages more useful when we fail to open a file. The difference with `map_err` is that one can still obtain the underlying error from the context if need be. Though surprisingly, the normal `Display` for a `Context` doesn't include the original error, so we essentially have to do a prefix here (see [1]). Before: ``` error: Failed to load YAML treefile: No such file or directory (os error 2) ``` After: ``` error: Failed to load YAML treefile: Can't open file "treecompose-post.sh": No such file or directory (os error 2) ``` [1] rust-lang-deprecated/failure#182 Closes: #1735 Approved by: cgwalters
This uses the
Context
feature of the failure crate to make errormessages more useful when we fail to open a file. The difference with
map_err
is that one can still obtain the underlying error from thecontext if need be. Though surprisingly, the normal
Display
for aContext
doesn't include the original error, so we essentially have todo a prefix here (see [1]).
Before:
After:
[1] rust-lang-deprecated/failure#182