Closed calavera closed 1 month ago
I think this is a overall pretty good trade-off.
IntoDiagnostic
and Into<Diagnostic>
. Wouldn't it be possible to just use IntoDiagnostic
for everything?Thanks for the feedback @TethysSvensson
I am a bit confused about why you would need to use both IntoDiagnostic and Into
. Wouldn't it be possible to just use IntoDiagnostic for everything?
It would be possible, but then we'd break even more people 🙈 I would recommend implementing Into<Diagnostic>
over using IntoDiagnostic
as a more gradual implementation change, and using IntoDiagnostic
when you cannot implement Into<Diagnostic>
like it's the case with external error libraries.
It would be possible, but then we'd break even more people 🙈 I would recommend implementing
Into<Diagnostic>
over usingIntoDiagnostic
as a more gradual implementation change, and usingIntoDiagnostic
when you cannot implementInto<Diagnostic>
like it's the case with external error libraries.
Actually on re-visiting this, I am not even sure I understand the purpose of the IntoDiagnostic trait at all. It seems to do the exact same thing as Into<Diagnostic>
, except it also has (feature-gated) impls for anyhow, eyre and mietre.
Wouldn't it be possible to get rid of IntoDiagnostic instead and instead do feature-gate a From<anyhow::Error> for Diagnostic
?
Wouldn't it be possible to get rid of IntoDiagnostic instead and instead do feature-gate a
From<anyhow::Error> for Diagnostic
?
Yeah, you're right. For some reason I thought it was not possible, but it totally is. I made the update and things look much more simple now.
Hi, thanks for improving this, looks a lot clearer and better to me. Docs helped me as a naive user a lot to understand the point of the breaking change.
One thing confuses me still though, the impl<T> From<Box<T>> for Diagnostic where T: std::error::Error
blanket impl. Why should it be impossible to convert some T: std::error::Error
but possible for Box<T> where T: std::error::Error
? This was already my primary confusion when the breaking change came up. Returning an error didn't work anymore, but slapping a Box
on it would fix it. But what has a Box
to do with the error handling? This blanket impl makes it also impossible to implement impl From<Box<MyErrorType>> for Diagnostic
(just tried with the thiserror example).
I think, it would make more sense to implement neither. And if the user really wants to use the simpel catchall conversion it is possible to go through Box<dyn std::error::Error>
. That makes it explicit that you get to an impl by going through the trait object, not by slapping on a Box
.
I think, it would make more sense to implement neither. And if the user really wants to use the simpel catchall conversion it is possible to go through
Box<dyn std::error::Error>
. That makes it explicit that you get to an impl by going through the trait object, not by slapping on aBox
.
Thanks for the feedback @tim3z. I agree with you. I've removed the implementation.
📬 Issue #, if available:
Fixes #901
✍️ Description of changes:
Change how we handle error types to be more ergonomic:
Replace Cow with String: Diagnostics are serialized into JSON as soon as the function returns, which means that their value is copied right away. The performance improvement of using Cow is minimal in this case, but it has some ergonomic implications because we have to handle their lifetimes. By removing the explicit lifetimes, people can return Diagnostic values with static lifetimes which was not possible before.
Add features to implement
From<T> for Diagnostic
for anyhow, eyre, and miette error types. This helps people that use those creates to transform their errors intoDiagnostic
without double boxing their errors.🔏 By submitting this pull request
cargo +nightly fmt
.cargo clippy --fix
.