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

Returns the same result for [1,2,3] and [3,2,1] (all arrays are sorted) #5

Open
jcward opened this issue Oct 12, 2017 · 3 comments
Open

Comments

@jcward
Copy link

jcward commented Oct 12, 2017

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?

jcward added a commit to jcward/crimp that referenced this issue Oct 12, 2017
Sort the hashes, not every array. Maybe not the prettiest fix, but it works.
@samfrench
Copy link
Contributor

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:

Creating an md5 hash of a number, string, array, or hash in Ruby

The main method used is signature which should provide a unique MD5 value. This is the main reason for sorting the array first.

Crimp.signature([1, 2, 3])
=> "612ad94f1618ae0c8f29f7c67d23022a"
Crimp.signature([3, 2, 1])
=> "612ad94f1618ae0c8f29f7c67d23022a"

I question why stringify is a public method as this should ideally be private as this is an internal to how we generate the unique hash value.

@jcward
Copy link
Author

jcward commented Oct 13, 2017

Then I'd think it should state:

Creating an md5 hash of a number, string, list, or hash in Ruby

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".

@kenoir
Copy link
Contributor

kenoir commented Nov 7, 2017

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?).

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

No branches or pull requests

3 participants