Tidy up error implementations #515
Conversation
| } | ||
| (Some(msg), None) => write!(f, "{msg}\nLocation: {}", self.location), | ||
| (None, Some(source)) => write!(f, "{source}\nLocation: {}", self.location), | ||
| (None, None) => write!(f, "\nLocation: {}", self.location), |
There was a problem hiding this comment.
@tomhoule i've copied the original implementation here, but i suspect the leading newline here is not intentional?
There was a problem hiding this comment.
Probably not. Ideally we'd have tests for the display impl.
There was a problem hiding this comment.
I can add those before this lands
| @@ -17,3 +17,4 @@ quote = "^1.0" | |||
| serde_json = "1.0" | |||
| serde = { version = "^1.0", features = ["derive"] } | |||
| syn = "^1.0" | |||
| thiserror = "2.0.11" | |||
There was a problem hiding this comment.
I'd prefer to avoid adding dependencies that are not 100% needed in this library. I like thiserror for application code, but I wouldn't use it there.
There was a problem hiding this comment.
Are you possibly confusing 'thiserror' and 'anyhow'? Thiserror targets libraries, not applications. It's a compile-time-only dependency- not exposed in public interface (it generates the boilerplate you would otherwise write by hand)
There was a problem hiding this comment.
It still adds to the compile times and the dependencies to audit. It's a good crate, but I wouldn't use it for a couple of modules in a library. Arguably, it's not worse than serde-derive which we already have in there, but I'm not sure why, we should remove if possible.
No description provided.