r2d2::Error should derive PartialEq and Clone

[OmegaJak]

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.

[sfackler]

Very few error types in the ecosystem implement those traits. You'll probably have a hard time with that design more broadly.

[OmegaJak]

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.

[sfackler]

It prevents this error type from ever including something not Clone or not PartialEq.

[OmegaJak]

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: https://github.com/serde-rs/json/issues/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.