speps / go-hashids

Go (golang) implementation of http://www.hashids.org
MIT License
1.32k stars 109 forks source link

Hex #41

Closed hypnobrando closed 6 years ago

hypnobrando commented 6 years ago

Created EncodeHex and DecodeHex functions for converting a hexadecimal string to a hashid and then back.

hypnobrando commented 6 years ago

@speps ping

hypnobrando commented 6 years ago

@speps ping

speps commented 6 years ago

Thanks a lot for your contribution! However, I'm questioning the usefulness of it? It seems like it's more helper functions than anything else and could stay in your own code base given that it's just a few lines of trivial code. I'm not opposed to including it but I want to know what's the use case, have more unit test coverage (what's the expected string, with or without 0x, etc.), and documentation.

hypnobrando commented 6 years ago

Hi @speps thank you for getting back. As you can see here, other hash id libraries have similar functions for converting a hex string to a hash id and back:

I'm sure other libraries have implemented these too.. I just haven't looked elsewhere.

The most popular use case is for converting between a mongo bson id to a hash id. I'm using the encode_hex function in my ruby services, and so I need a similar function in my go services in order to decode_hex. I'm sure there are others out there that are running into the same sort of problem.

hypnobrando commented 6 years ago

anyways, if you think this is worth merging, I'll happily create more unit tests first... just let me know

hypnobrando commented 6 years ago

hi @speps just wanted to check in. would you like me to write unit tests for this?

speps commented 6 years ago

Yes please, but most importantly documentation. See how the others are documented, you need to specify how the methods expect arguments to be formatted.

dolmen commented 6 years ago

By looking at the unit test the use case of this PR seems to encode a cryptographic hash result such as MD5 or SHA1. The result of such hashing is a []uint8. There no need to convert it first to hex and then convert from hex to hashid.

So I'm very disappointed that this has been merged. This library should stay lightweight. But instead it gets more and more garbage. I want more and more to fork it.

dolmen commented 6 years ago

Besides that, the encoding is really awkward: "0123456789abcdef" is encoded as []int{16, 17, 18, 19, 20, 21, 22, 23, 24, 25, 26, 27, 28, 29, 30, 31} before being passed to Encode. This is pure insanity. And this encoding is absolutely not documented (not even a reference to the python or ruby original implementation from which @brandoneprice31 suggests this crack comes from).

speps commented 6 years ago

I wouldn't mind giving ownership to someone else @dolmen to be honest. I do agree it should be less bloated, I'll see what I could do for 2.0 but if you have some time, I wouldn't mind a cleanup PR to get us to 2.0.

dolmen commented 6 years ago

DecodeHex also doesn't properly check that decoded numbers are correctly in range [16, 31].