Make types eagerly implement common traits #53
Conversation
src/mock_stream.rs
Outdated
| @@ -1,6 +1,7 @@ | |||
| use std::io::{Error, ErrorKind, Read, Result, Write}; | |||
| use std::cmp::min; | |||
|
|
|||
| #[derive(Copy, Clone, Debug, Eq, Ord, PartialEq, PartialOrd, Hash)] | |||
There was a problem hiding this comment.
MockStream is not exposed outside the crate, and there's no reason for it to derive these traits.
There was a problem hiding this comment.
Also, MockStream cannot implement Copy, since it contains Vecs.
There was a problem hiding this comment.
Fixed the wrong Copy derivation. I tested locally but apparently something went wrong, sorry!
There was a problem hiding this comment.
I get your point but, these traits should be implemented eagerly, the point of the recommendation is to avoid needless discussion and abitrary decisions on what traits are and aren't ‘useful’. If I want to debug tests I could well have a use for many of these traits.
There was a problem hiding this comment.
I read the recommendation as being about the crate's API (and hence, about the types that are public). The reason I am hesitant about eagerly adding these derives to internal types is that it unnecessarily limits what we can put inside them for very little gain. For externally exposed types, I totally see the advantage of eagerly implementing these traits, since the overhead for users for adding impls is very high. However, for internal types, it's trivial to add derives on-demand.
| @@ -16,6 +16,7 @@ const CR: u8 = 0x0d; | |||
| const LF: u8 = 0x0a; | |||
|
|
|||
| /// Stream to interface with the IMAP server. This interface is only for the command stream. | |||
| #[derive(Debug)] | |||
There was a problem hiding this comment.
This seems fine, though I'm unsure why having the client implement Debug is useful?
There was a problem hiding this comment.
One other advantage of implementing traits eagerly, i.e. if there is no deliberate reason or cause not to, is that a compile time error will occur if the type is extended to include elements/fields that do not implement that common trait. Then the breaking change is pointed out and it can be reconsidered.
| @@ -1,6 +1,6 @@ | |||
| use std::fmt; | |||
|
|
|||
| #[derive(Eq, PartialEq)] | |||
| #[derive(Clone, Debug, Eq, Ord, PartialEq, PartialOrd, Hash)] | |||
There was a problem hiding this comment.
I don't think it's meaningful to implement Ord or PartialOrd for Mailbox.
There was a problem hiding this comment.
See previous comments.
There was a problem hiding this comment.
I agree in general (again, since this is an externally expose type). However, I don't think we want to commit to Mailbox being Ord. That seems like a strong, but ultimately not particularly useful guarantee. And eagerly adopting it would mean that we are committing to continuing to support Mailbox: Ord unless we bump the major version number.
There was a problem hiding this comment.
No you wouldn't commit to anything yet, see SemVer.
There was a problem hiding this comment.
Implementing common traits should be done eagerly and we will be done with it. I see where you're coming from that you have reasons not to implement Ord but those should then be made explicit. Can you give a reasonable example where the type could not be Ord anymore? As I motivated in my earlier comment, adding constraints to types by implementing common traits is not just a potential disadvantage, it's an advantage since you will are forced to modify the type more carefully. The proper strategy IMO:
- Before 1.0.0: eagerly implementing traits for all types as the Rust API Guidelines prescribe.
- At 1.0.0: making final decisions on what traits to unimplement, based on an explicit motivation and considering the actual downstream effects.
Please, go ahead and make any changes to this branch as you see fit. This PR is holding up potential further contributions I wanted to make.
There was a problem hiding this comment.
I guess I consider this crate stable enough that it should really be 1.0.0, at which point these would be committing. I can absolutely see Mailbox gaining fields that are not Ord, such as pointers back to the Client for further operations on the selected mailbox, which is why I'm hesitant for that to be added. Again though , the other traits seem fine to derive.
I think we'll need @mattnenterprise to look at this.
|
@jonhoo: do you have time to adapt/merge this, is something else blocking a merge? |
|
As I mentioned in #53 (comment), I think |
|
I still disagree with some of the other derives (especially for |
|
Yeah I understand, but I had commented: feel free to adapt the PR as you see fit. You can push new commits as maintainer. This seems like a better approach than to keep on coordinating all the details with me. 🙂
From: Jon Gjengset
Sent: maandag 30 oktober 2017 08:10
To: mattnenterprise/rust-imap
Cc: Sander Maijers; Author
Subject: Re: [mattnenterprise/rust-imap] Make types eagerly implement commontraits (#53)
As I mentioned in #53 (comment), I think Ord and PartialOrd should be removed from Mailbox, and arguably also from MockStream. I see your arguments the other way too, but disagree with them. Which is why I'd like @mattnenterprise's input on this. Alternatively, if you remove those derives (which I don't think should matter for any other changes you have pending), I'd be happy to merge.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub, or mute the thread.
|
See https://rust-lang-nursery.github.io/api-guidelines/interoperability.html#types-eagerly-implement-common-traits-c-common-traits