feat: implement AsRef<[u8]> for Atom - #303
Conversation
mrobinson
left a comment
There was a problem hiding this comment.
Seems fine to me except that the tests are failing to compile, which probabaly indicates API breakage.
|
Ah shoot, I tested integration-tests but must have missed the built-in tests. I'll fix and push when I'm back in front of a computer. And yes, technically this is a breaking change as direct calls to as_ref() will now be ambiguous. If this is a deal-breaker I'm happy to take the L and close this. |
I think this isn't a problem in-and-of itself, but will require us to do a minor version bump. @SimonSapin Do you mind also taking a look at this one? |
| pub fn as_str(&self) -> &str { | ||
| AsRef::as_ref(self) | ||
| } | ||
|
|
There was a problem hiding this comment.
It should all optimize away but I’d prefer going through fewer layers of abstraction. The actual logic is in the Deref impl so, here and in the new as_bytes method let’s use this:
| pub fn as_str(&self) -> &str { | |
| AsRef::as_ref(self) | |
| } | |
| pub fn as_str(&self) -> &str { | |
| self // auto-deref | |
| } |
There was a problem hiding this comment.
Good catch! I'm actually just going to implement all of the logic in as_str as as_bytes, and then the traits can call into these. I think that'll make the code much easier to follow.
| pub fn as_bytes(&self) -> &[u8] { | ||
| AsRef::as_ref(self) | ||
| } |
There was a problem hiding this comment.
| pub fn as_bytes(&self) -> &[u8] { | |
| AsRef::as_ref(self) | |
| } | |
| pub fn as_bytes(&self) -> &[u8] { | |
| str::as_bytes(self) | |
| } |
5f4d135 to
b299235
Compare
|
Note that per semver spec, since the major version of this crate is currently 0, you don't need to bump to version 1 as anything is allow to break at any time. That said, I get that a) that can be annoying for down-stream users, and b) this crate seems pretty stable so a 1.0 may be worthwhile anyway. |
|
This crate has public dependency on phf (which is not 1.0). So it might be a bit premature to make it 1.0 |
That’s what semver.org says but not what cargo does. For example if a
@mrobinson I assume you meant an incompatible bump from 0.10.0 to 0.11.0, as opposed to 0.10.1? |
Yep, I meant a bump to the next minor version in the (major, minor, micro) triple. |
Also adds disambiguating functions as_str() and as_bytes() to mirror std apis
b299235 to
6611088
Compare
|
Thank you! |
Also adds disambiguating functions as_str() and as_bytes() to mirror std apis