fix(orderBy): changes orderBy to use case sensitive comparison to break ...

[deathbob]

...ties on strings

Previously running orderBy on an array of strings such as ['A cow', 'a cow'] would
result in ['A cow', 'a cow'], regardless of the value of reverseOrder. This is because orderBy cast
all strings to lowercase before comparing them.

This commit changes the behavior to result in ['a cow', 'A cow'] when reverseOrder is true,
and ['A cow', 'a cow'] when reverse order is false.

[mary-poppins]

Thanks for the PR! Please check the items below to help us merge this faster. See the contributing docs for more information.

• Uses the issue template (#8112)

If you need to make changes to your pull request, you can update the commit with git commit --amend.
Then, update the pull request with git push -f.

Thanks again for your help!

[deathbob]

Hi @mary-poppins I tried to use the issue template, but it wanted to read and write all public and private repo data which I am not comfortable with.

How else can I get an issue logged for this, happy to do anything short of handing complete control of all my code :)

[Narretz]

Don't worry, the issue template isn't absolutely necessary.
I wonder if this change makes more sense as an option to the filter, because case sensitive sorting may have a performance impact on large sets.

[deathbob]

@Narretz Good call, it was naive to do the toLowerCase() call two extra times.

I pushed a change to optimize that, and benchmarked it against the previous code.
(code here) https://gist.github.com/deathbob/f2024bdf9171f52e65e1

For an array of 100,000 objects, each with a name property of 10 random characters, the code on master branch took an average of 1586ms, the code in this PR took an average of 1545ms.

So I think performance is now on par with master, and shouldn't be a problem.

[shahata]

This is duplicate code... Why not do:

var v1Lower = v1.toLowerCase();
var v2Lower = v2.toLowerCase();
if (v1Lower !== v2Lower) {
  v1 = v1Lower;
  v2 = v2Lower;
}

[deathbob]

Works for me!

[deathbob]

@shahata pushed your suggested changes.

[deathbob]

@Narretz @mary-poppins @shahata anything else I can do to help get this merged?

[caitp]

If you want to get this merged, a good start would be fixing the lint errors which are causing Travis to fail. You can test those locally with grunt ci-checks.

However, I'm not big on this change --- particularly, it's very strange to have the default behaviour be case insensitive in some ways, and case sensitive in other ways.

The orderBy filter allows you to provide a custom comparator, so any kind of case-sensitivity you want to provide is easily pluggable, so I'm not really sure what this is really adding, other than making the behaviour inconsistent. Maybe its' a "good" inconsistency that people would be happy with, but maybe it's not, I'm not sure.

[deathbob]

Thanks @caitp, pushed fix for the lint error.

The orderBy filter allows you to provide a custom comparator, so any kind of case-sensitivity you want to provide is easily pluggable

I don't think it is possible to provide a custom comparator, only a custom 'getter' function. The comparator is always the built in (case-insensitive) one.

As I noted above this change doesn't introduce any performance penalty, and the benefit is correctness and consistency. On master currently

   var array = [{name: "John"}, {name: "john"}];
   var asc = orderBy(array, 'name', true);
   var desc = orderBy(array, 'name', false);
   expect(asc).toEqualData(desc.reverse());

// Error: Expected [ { name : 'John' }, { name : 'john' } ] to equal data [ { name : 'john' }, { name : 'John' } ].

As you can see, regardless of the value of the reverseOrder parameter, the return value of orderBy is the same, which seems consistent to a fault 😃

For what it's worth I have actual users in an actual app who are complaining to me that reversing the sort order of a list does not exactly reverse the list. It's because of them that I am on this quest to make the reverseOrder toggle really truly reverse the output of orderBy. And it's because I haven't been able to figure out any easy workaround that I'm proposing a PR, instead of just doing something simple in my app to make them happy.

[caitp]

I don't think it is possible to provide a custom comparator, only a custom 'getter' function. The comparator is always the built in (case-insensitive) one.

The predicate parameter.

[caitp]

Actually, I might be wrong about that, lets find out!

[caitp]

yeah, we only have a custom comparison operator for the filterFilter, not orderBy. That's probably the right way to implement this, IMO. But an alternative predicate notation could be used too, maybe.

[deathbob]

@caitp I'm not sure accepting a custom comparator is necessary because unlike filtering, ordering has a commonly understood definition.
If you hand a kid a list of names and say "Filter these names" they'll probably reply "Um... How?". (needs a custom comparator to do anything).
If you hand a kid a list of names and say "Order these names" they'll probably say "OK" and put them in ascending alphabetical order. A real smart-ass might put them in order from shortest to longest, but still the implication and expectation of "order" is commonly understood.

Angular does well with that as the common expectation, it gets the ascending alphabetical order correct out of the box.

Where things go off the rails IMHO is the common expectation that there is some sort of total order, such that sorting a list in descending order is the "opposite", or reverse, of sorting a list in ascending order; that the order of elements in a sorted list depends only on their identity and their relationship to other elements in the list, not to the order they were inserted in the (original, unsorted) list and not to their identity after being secretly run through some other function.

Angular fails in that expectation, and this PR is a remedy. A very small and performance neutral remedy.
It has the benefits of introducing a real and true total ordering over the set of strings, and making a small group of humans who have to use a small angular app in their job, happy.

[caitp]

It's not that simple, actually. The predicate array does a bit to help customize the way these comparisons work, but custom comparator functions would actually do this a lot better --- we'd be easier on the callstack, we'd be simpler to implement.

I think we should see what Igor thinks about this one, because this is a pretty minor change, but I'm not certain it's necessarily the "right" change.

[deathbob]

@IgorMinar can we get your thoughts here?