Conversation
fischeti
force-pushed
the
fischeti/error-handling
branch
6 times, most recently
from
f9fa419 to
af269e3
Compare
fischeti
force-pushed
the
fischeti/error-handling
branch
from
af269e3 to
fc383f5
Compare
fischeti
force-pushed
the
fischeti/error-handling
branch
from
fc383f5 to
9cd009e
Compare
micprog
left a comment
micprog
left a comment
There was a problem hiding this comment.
Love the upgrade, there look to be some nice usability and styling improvements! I have some questions about some of the implementation, see comments.
|
|
||
| use crate::error::*; | ||
| use crate::Result; | ||
| use crate::infoln; |
There was a problem hiding this comment.
This is more of a question here, not something for this PR but future: would switching infoln to use log::info instead make sense?
| Err(err!( | ||
| "{}.core already exists, please delete to generate.", | ||
| name | ||
| )))? | ||
| ))? |
| .map_err(|e| Error::new(format!("Cannot create output file: {}", e)))?, | ||
| ), | ||
| Some(path) => { | ||
| Box::new(File::create(path).map_err(|e| err!("Cannot create output file: {}", e))?) |
There was a problem hiding this comment.
Isn't this better handled with wrap_err_with?
| .render_str(template, &tera_context) | ||
| .map_err(|e| { Error::chain("Failed to render template.", e) })? | ||
| .into_diagnostic() | ||
| .wrap_err("Failed to render template.")? |
There was a problem hiding this comment.
What's the difference in wrap_err and wrap_err_with?
| } | ||
|
|
||
| Err(Error::new( | ||
| Err(err!( |
There was a problem hiding this comment.
I think this can also be a bail!?
| let warning = Warnings::IgnoredPath { | ||
| cause: cause.to_string(), | ||
| }; | ||
| warning.emit_or_error().err().map(Err) |
There was a problem hiding this comment.
Just curious how the err().map(Err) works, and if it preserves the Some/None behavior.
| })?, | ||
| Some(d) => d | ||
| .validate(vctx) | ||
| .map_err(|(key, cause)| cause.wrap_err(format!("In override `{key}`.")))?, |
There was a problem hiding this comment.
Is the map_err -> wrap_err leveling necessary, or is a single wrap_err possible?
| let warning = Warnings::IgnoredPath { | ||
| cause: cause.to_string(), | ||
| }; | ||
| warning.emit_or_error().err().map(Err) |
| let warning = Warnings::IgnoredPath { | ||
| cause: cause.to_string(), | ||
| }; | ||
| warning.emit_or_error().err().map(Err) |
| Some(d) => d | ||
| .validate(vctx) | ||
| .map_err(|(key, cause)| Error::chain(format!("In plugin `{}`:", key), cause))?, | ||
| .map_err(|(key, cause)| cause.wrap_err(format!("In plugin `{key}`.")))?, |
2 participants
The previous error implementation is outdated: both
causeanddescriptionhave been deprecated since a while in the standard library. This PR modernizes the error type and handlingAdopting
mietteerror typeThe new global error type is now
miette::Report, which has the following benefits:Diagnostics.miette::Reporthas neat macros likebail!,ensure!andmiette!(inspired byanyhow)wrap_err(resp.wrap_err_withfor lazy evaluation), which additionally has the advantage that the source is retained with the standardsource()which allows to traverse the whole chain of errors and print it out.Suppressible errors
An additional
emit_or_error()function has been introduced to support downgrading errors to warnings. The suppressible error is still defined as aWarninge.g. with codeW30, but the function will check whetherE30has been suppressed and change the severity/code and returnErr. Otherwise, it will emit the warning as usual and returnOk. This cleans up the usage of suppressible errors in the code:Restructuring
Additionally
error.rshas been removed entirely, and the rest (e.g.stageln!and other macros) moved to thediagnostic.rs, where they also belong imo.