Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add `Clone` + `PartialEq` traits to `bincode::ErrorKind` #273

Closed
wants to merge 1 commit into from

Conversation

@SamuelMarks
Copy link

SamuelMarks commented Jul 22, 2019

Add Clone trait for bincode::ErrorKind. Also add PartialEq trait to allow use assert_eq! in tests.

@SamuelMarks SamuelMarks changed the title add Clone trait to bincode::ErrorKind Add `Clone` & `PartialEq` traits to `bincode::ErrorKind` Jul 22, 2019
@SamuelMarks SamuelMarks changed the title Add `Clone` & `PartialEq` traits to `bincode::ErrorKind` Add `Clone` + `PartialEq` traits to `bincode::ErrorKind` Jul 22, 2019
@jdm
Copy link
Member

jdm commented Jul 22, 2019

It's not clear to me why the 1.18 build for Travis CI is failing. I'm also not thrilled by the idea of a PartialEq implementation that lies about Io errors

@SamuelMarks
Copy link
Author

SamuelMarks commented Jul 22, 2019

It's difficult to decide between ways of handling errors, and creating large Rust projects that can propagate errors of any type.

Happy to make changes to get it approved by you. Tell us what to do.

FYI: That IO line is same as https://github.com/spacejam/sled/blob/3b20bf4/crates/pagecache/src/result.rs#L103

@jdm
Copy link
Member

jdm commented Jul 22, 2019

Unless there is a stronger motivation for PartialEq than writing tests, I would prefer not to include that change.

@dtolnay
Copy link
Collaborator

dtolnay commented Jul 22, 2019

I would prefer to pass on the Clone impl as well. In code that needs multiple owned values of the same error, it should use Rc<bincode::Error> or similar. As an advantage over the impl in this PR, that approach preserves the cause of the io error. It would be surprising for Clone to produce an error that behaves differently from the original.

@dtolnay dtolnay closed this Dec 18, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.