Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix EncodeHex and DecodeHex from original implementations #51

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

mashmooli
Copy link

Hi
in original implementation encode the hex 5a74d76ac89b05000e977baa with salt this is my salt is equal with bv89jEY45DslgBOeD2Qg. but i tested it with this repo and it's different with original implementation.
i fixed it in this PR.

hashids_test.go Outdated
@@ -73,7 +73,7 @@ func TestEncodeDecodeEpoch(t *testing.T) {

func TestEncodeDecodeHex(t *testing.T) {
hdata := NewData()
hdata.MinLength = 30
hdata.MinLength = 8
Copy link

@ghost ghost May 21, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do you change the MinLength? What is the result if you leave it at 30? If you change the implementation, leave the unit test unchanged.

Copy link
Author

@mashmooli mashmooli May 21, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks for replying. i had turn backed it to 'MinLength = 30'
and you can check it with js example in: https://codepen.io/anon/pen/arVLpG

@bbsbb
Copy link

bbsbb commented Aug 20, 2019

What is the status of this? Is it going to be merged or not? Also, if it gets merged, how is it going to be released, seeing that it breaks backward compatibility?

@ghost
Copy link

ghost commented Aug 20, 2019

I propose to close this issue.

@vtolstov
Copy link

vtolstov commented Apr 5, 2020

why not release never version with go mod as /v2 ?

@dolmen
Copy link
Collaborator

dolmen commented Oct 19, 2020

Well, I think that a /v3 (v2.0.0 is already tagged) would have a lighter API and drop EncodeHex and DecodeHex.

In addition, this "fixed" implementation of DecodeHex use a regexp, which is not efficient, so it must not be merged as is.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants