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

Don't compare lowercase versions of strings in naturalCmp #326

Closed
megawac opened this issue Nov 21, 2014 · 3 comments · Fixed by #339
Closed

Don't compare lowercase versions of strings in naturalCmp #326

megawac opened this issue Nov 21, 2014 · 3 comments · Fixed by #339

Comments

@megawac
Copy link
Contributor

megawac commented Nov 21, 2014

This is pretty inconsistent with other implementations such as PHPs and python's natsort

(I'm using a fork of your lib while these issues are being resolved)

megawac added a commit to megawac/underscore.string that referenced this issue Nov 21, 2014
@esamattis
Copy link
Owner

I'd like it be consistent with the others too but I wonder if this is a bug or feature. Meaning do we need major version release if we change this.

@stoeffel
Copy link
Collaborator

Yes I think we need a major release for this. Maybe someone was relaying on array.sort(_.str.naturalCmp) to not change the order of strings with only different capitalisation.

@stoeffel
Copy link
Collaborator

After a quick search,I found only one example where someone was using naturalCmp in a case insensitive sort.

https://github.com/NicoleRauch/NodeTestingTechniques/blob/202535d31fea98a3cfcf843f9754a3b998ff2ec0/sources/HTMLtests/problem/lib/members/memberstore.js

var _s = require('underscore.string');

var sortCaseInsensitive = function (objectlist) {
  return objectlist.sort(function (a, b) {
    return _s.naturalCmp(a.lastname + ' ' + a.firstname, b.lastname + ' ' + b.firstname);
  });

};

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 a pull request may close this issue.

3 participants