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
201 stars 55 forks source link

remove traits #28

Closed Luro02 closed 4 years ago

Luro02 commented 4 years ago

I think the point of FromHex and ToHex is to allow other librarys to accept any kind of type, that implements those traits (but why? to decode or encode them? why not do that before passing them to the function?).

The encoding and decoding traits should be symmetric (currently they aren't), by that I mean that any type, that can be encoded should be decodable too.

The encode and decode function can only work with an input of Iterator<Target = u8> or Iterator<Target = char> (where the char can be converted to an u8...), this includes&[u8], String, &str or [u8; n].

Therefore it might be better to provide a single trait like this

trait ToBytes {
    fn to_bytes(&self) -> Iterator<Target = u8>;
}

and make the hex function signatures like this

fn encode<T: ToBytes>(value: T)

I omitted the return type, because I don't know what type should be returned. If you are too generic it can no longer be used with for example no_std, or it makes the code slow/too complicated.

There is also some duplication inside the library, like several encode functions? (not sure, it could also have been decode)

Overall I think it would be better to just remove them.

Luro02 commented 4 years ago

The docs say, that you should use the traits, if you need a bit more control; If you need a bit more control, use the traits ToHex and FromHex instead.

This could achieved through a struct like this;

Decoder::new()
     .validate_ascii(true)
    .ignore_prefix(true) // ignores `0x`/`0X`
    .decode(b"0xFFFFFFF")?;

and an equivalent for the encoder

LukasKalbertodt commented 4 years ago

I think the point of FromHex and ToHex is to allow other librarys to accept any kind of type, that implements those traits

I think they are primarily here for the encode and decode functions. And for those, the argument "just encode/decode it before" obviously does not make sense.

The encoding and decoding traits should be symmetric (currently they aren't), by that I mean that any type, that can be encoded should be decodable too.

That doesn't quite work. All types implementing FromHex have to be owned types (i.e. Vec or arrays), whereas encoding something only needs a reference, so &[u8] and &str are fine, too. Furthermore, one could implement FromHex for String, but this assumes that the encoded data is a valid UTF8 string. What would one do when it isn't? Panic? That doesn't sound like a good solution to me. Instead, users can simply use standard library functions to turn a Vec<u8> into a String with or without checking.

Overall I think it would be better to just remove them.

If you want to and have the time, you could basically rewrite this crate as you think it would be better. Of course, I doubt such a PR would be accepted like that, but it would be useful to check your own assumptions and show us exactly what you have in mind. I'm only suggesting that because this crate is pretty small, so even if none of your code is used in the end, you have not wasted a lot of time.

I don't quite get what exactly you are proposing and how this design would solve anything. But maybe you could convince people with more details.

Luro02 commented 4 years ago

My bad, I somehow didn't realize, that those traits are meant for structs and not for function bounds -.-

What I thought they would be used for

fn decode<T: FromHex>(value: T);