r2d2::Error should derive PartialEq and Clone
OmegaJak opened this issue · 4 comments
Maybe there's a good reason not to do this, but in my use case, it would be very convenient if Error derived PartialEq and Clone. I'm wrapping r2d2::Error in my own Error type, which I would like to be able to derive PartialEq and Clone for. PartialEq, particularly in combination with Debug, is particularly useful in tests, where it allows you to assert_eq!(Err(...), result). Sure, you can assert!(...) with a custom message, but I think it's more ergonomic and produces better errors to use assert_eq!. And while adding code just to support tests isn't something you generally should do, in this case, I don't see it increasing maintenance burden at all.
It should just be as simple as adding PartialEq and Clone to the #[derive()] for r2d2::Error.
Any good reason not to add them? If not, I'm happy to raise the (tiny) PR making this change.
Very few error types in the ecosystem implement those traits. You'll probably have a hard time with that design more broadly.
Is that a good reason not to, though?
If you think it would harm the library in any way, then sure. But otherwise, with such a simple error type, I don't see any reason to restrict possible uses of it by not deriving.
And yes - wrapping generally isn't preferred. But sometimes it's a nice middle ground for internal code between properly embedding the contents of the error while adding context (like you ideally would in a library use case) and resorting to something like anyhow for application code.
Anyway, if you still disagree, I understand. Feel free to close and I'll work around it by embedding a to_string() of r2d2's error.
It prevents this error type from ever including something not Clone or not PartialEq.
Very true.
It occurred to me to check what serde does, and it doesn't implement PartialEq either. That led me to this issue with some relevant discussion: serde-rs/json#271, with one of the downstream suggestions being to use assert_matches!. This is better for all parties - ergonomic tests, good errors, and no need to derive just for tests.
Closing this issue as not planned.