-
Notifications
You must be signed in to change notification settings - Fork 5
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
Returns the same result for [1,2,3] and [3,2,1] (all arrays are sorted) #5
Comments
Sort the hashes, not every array. Maybe not the prettiest fix, but it works.
Hi @jcward, I would say the sorting of the arrays is intended for Crimp. The main purpose of Crimp (taken from the main GitHub page) is to:
The main method used is Crimp.signature([1, 2, 3])
=> "612ad94f1618ae0c8f29f7c67d23022a"
Crimp.signature([3, 2, 1])
=> "612ad94f1618ae0c8f29f7c67d23022a" I question why |
Then I'd think it should state:
For me, order of elements in an array matters, and this is a surprise. Again, this issue is about letting users know what they're getting (not to mention the PR's to fix it, if it's not what they want.) Feel free to close as "as designed". |
I'm pretty sure this behaviour was intended. I'm not sure if you're going to need to be careful about backwards compatibility here (i.e. where have IDs this thing has generated ended up? Are they still used?). |
The self.parse_array function sorts all arrays. That seems like odd behavior, but perhaps that's intended?
Crimp.stringify([1,2,3])
->[\"1Fixnum\", \"2Fixnum\", \"3Fixnum\"]Array
Crimp.stringify([3,2,1])
->[\"1Fixnum\", \"2Fixnum\", \"3Fixnum\"]Array
I presume the sort is to fix hash order problems, but perhaps better would be to explicitly sort just the hashes?
The text was updated successfully, but these errors were encountered: