search

KokaKiwi / rust-hex

A basic crate to encode values to hexadecimal representation. Originally extracted from rustc-serialize.
https://crates.io/crates/hex
Apache License 2.0
203 stars 57 forks source link

Make this crate a in-place replacement for `rustc-serialize` #9

Closed dpc closed 6 years ago

dpc commented 7 years ago

Impl ToHex for ?Sized patch is actually general bugfix, IMO.

Please let me know if you agree with the changes. One of my motivations was to use it as an in-place replacement for rustc-serialize that got deprecated.

I don't think playing with AsRef in the trait is any benefit. And the trait was only implemented for Vec<u8>...

LukasKalbertodt commented 7 years ago

I don't think this crate should mimic the exact API of rustc_serialize. After all, it was deprecated for good reasons. For example, its API was widely considered non-optimal. I also don't think the FromHex trait should work like the rustc_serialize one. From* traits usually work like the std::convert::From trait in that the type implementing it can be created from something (a hex string in this case). That's the reason it is only implemented for Vec<u8>. And with the current API, it does make sense to use AsRef, as it adds a lot of flexibility.

I proposed another API change in #11. It won't serve as a drop-in replacement for rustc_serialize (which shouldn't be the goal IMO), but it offers an easy to use API, which even matches the API of other crates, like base64.

I'm really sorry if this all just sounds like "I don't like your PR, look at my better PR", but I do really think that this crate shouldn't mimic rustc_serialize.

dpc commented 7 years ago

I haven't followed rustc_serializeAPI or discussion etc. All I know I have an existing code that is using rustc_serialize for hex conversion and I need to move it to something that is not deprecated. I attempted to use current rust-hex API and I had to change nicely looking .from_hex and .to_hex everywhere to some <FromHexShenanigans>. I wanted str/String support since when I work with hex, I work with text not Vec<u8> and it wasn't there, and ?Sized was broken. Then again, I might have missed something, and I didn't really do careful investigation. It's fine for me one way or another. I'll investigate the alternative API when I have some time. If it doesn't work for me, I can always publish the above PR as some rustc_serialize_hex_replacement and use that privately, so it's all fine with me. :)

KokaKiwi commented 6 years ago

I'm closing this PR as it has been "replaced" by #11 Maybe there's things which could be keep from this PR, but I think a new PR would be better