Replace thiserror with SNAFU #379
Conversation
Probably the better default than not requiring it.
| fs::write(&path, serialized).map_err(|io| SaveError::Write(path, io))?; | ||
| fs::write(&path, serialized).context(WriteSnafu)?; |
There was a problem hiding this comment.
What's the benefit of sanfu over thiserror? It seems like we're just exchanging map_err for a custom trait / .context but it provides the exact same utility in the end?
All the extra variants being added in this PR is great, but can just as easily be done with thiserror
There was a problem hiding this comment.
Two benefits:
- The
contextstuff is shorter, especially when adding extra context. This makes it practical to not implementFrom<UnderlyingError> for MyErrorand in factsnafu's derive macro never implements this. - As a consequence, it becomes more apparent where errors are being converted, so it is much easier to notice / review whether a particular error variant is overused – I've seen some
IoandNixerror variants, many of which would likely benefit from being split up.
There was a problem hiding this comment.
We can omit #[from] when defining variants with thiserror, but adding more friction here is definitely a good thing. The catch all Io, etc patterns are no good. If we can mostly solve that on the library portions of this repo then that'd be a big win, regardless of which error lib we use (I ultimately have no preference, they're effectively the same)
There was a problem hiding this comment.
I'm thankful you're looking at this, thank you! I'll jump back on matrix next week and it'd be good to connect on errors. It'd be great to focus on user facing errors next (moss / boulder CLI stuff) with anyhow / eyre. We're using thiserror there too and it's definitely the wrong approach.
There was a problem hiding this comment.
As an example I have this old draft:
If we decide to go this path, we should probably convert all of the crates.
This forces us to be much more explicit about error propagation and adding any relevant context, but makes that very convenient.