Skip to content

Fix EncodeHex and DecodeHex from original implementations#51

Open
mashmooli wants to merge 3 commits into
speps:masterfrom
mashmooli:master
Open

Fix EncodeHex and DecodeHex from original implementations#51
mashmooli wants to merge 3 commits into
speps:masterfrom
mashmooli:master

Conversation

@mashmooli

Copy link
Copy Markdown

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.

Comment thread hashids_test.go Outdated
func TestEncodeDecodeHex(t *testing.T) {
hdata := NewData()
hdata.MinLength = 30
hdata.MinLength = 8

@ghost ghost May 21, 2019

Copy link
Copy Markdown

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.

@mashmooli mashmooli May 21, 2019

Copy link
Copy Markdown
Author

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

bbsbb commented Aug 20, 2019

Copy link
Copy Markdown

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

ghost commented Aug 20, 2019

Copy link
Copy Markdown

I propose to close this issue.

@vtolstov

vtolstov commented Apr 5, 2020

Copy link
Copy Markdown

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

@dolmen

dolmen commented Oct 19, 2020

Copy link
Copy Markdown
Collaborator

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