Always return +-1 or 0 in naturalCmp

[megawac]

Fixes #323

[stoeffel]

This looks good to me.
I think all of the usecases are [str1, str2].sort(_.naturalCmp); or if (_.naturalCmp(str1, str2) >= 1), since I guess everyone who ever used naturalCmp thought it works this way anyway. So I think a minor release is okay.
But I think a description in the readme would be nice.

[megawac]

Most likely true, I only noticed this issue because it was breaking several of my unit tests for an alternate compare method which delegates to str.naturalCmp

[stoeffel]

I will test it more tomorrow and merge it then.

[stoeffel]

Hmm... it looks like it's not uncommon to return something else than 1, 0 and -1 from
a comparator.
Checkout https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/sort

function compareNumbers(a, b) {
  return a - b;
}

or http://stackoverflow.com/questions/17382091/javascript-sort-comparator-function

I don't think the current implementation is a bug and after rethinking it I would rather not merge this PR and risk breaking someone else's code.

I would recommend you to use Math.sign or a polyfill in your code if it is crucial to you to return 1,0,-1.

return Math.sign(str.naturalCmp(this.prerelease.join("."), other.prerelease.join(".")));

[megawac]

I'd say if not a bug its at least inconsistent. I'm on my phone so I'm not
going to provide examples but I have a hunch that someone doing

If (compare(a,b) == 1)

Would be more likely than someone doing

if (Compare(a,b) > 556)

Especially as I haven't seen this behavior reported before. I'm under the
impression that this may fix more broken edge than break conditions.

Also the other commits correct some slightly off logic
On Nov 22, 2014 12:04 PM, "Christoph Hermann" [email protected]
wrote:

Hmm... it looks like it's not uncommon to return something else than 1, 0
and -1 from
a comparator.
Checkout
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/sort

function compareNumbers(a, b) {
return a - b;
}

or
http://stackoverflow.com/questions/17382091/javascript-sort-comparator-function

I don't think the current implementation is a bug and after rethinking it
I would rather not merge this PR and risk breaking someone else's code.

I would recommend you to use Math.sign or a polyfill in your code if it is
crucial to you to return 1,0,-1.

return Math.sign(str.naturalCmp(this.prerelease.join("."), other.prerelease.join(".")));

—
Reply to this email directly or view it on GitHub
#324 (comment)
.

[stoeffel]

Yes you may have a point with that. And I like the "of topic" changes and the tests.
So if we agree that it isn't a bug, it goes into the next major release (2.5.0) together with issue #326, if we deside to change that too.
It would be great if you could add something to the README, to make it absolutely clear to users.

Thanks for the PR.