feat(orderBy): add support for custom comparators #14468

gkalpak · 2016-04-19T17:42:18Z

What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
Feature

What is the current behavior? (You can also link to an open issue here)
The comparator used by orderBy is hard-coded.

What is the new behavior (if this is a feature change)?
The user can specify a custom comparator function.

Does this PR introduce a breaking change?
No

Please check if the PR fulfills these requirements

The commit message follows our guidelines: https://github.com/angular/angular.js/blob/master/CONTRIBUTING.md#commit-message-format
Tests for the changes have been added (for bug fixes / features)
Docs have been added / updated (for bug fixes / features)

Other information:
With this change the 3rd argument is interpreted as a comparator function, used to compare the values returned by the predicates. Leaving it empty falls back to the default comparator and a value of false is a special case that also falls back to the default comparator but reverses the order. Thus the new implementation is backwards compatible.

This commit also expands the documentation to cover the algorithm used to sort elements and adds a few more unit and e2e tests (unrelated to the change).

Helps with #12572 (maybe this is as close as we want to get).

Fixes #13238
Fixes #14455

Closes #5123
Closes #8112
Closes #10368

Narretz · 2016-04-21T20:01:42Z

test/ng/filter/orderBySpec.js

@@ -157,6 +183,38 @@ describe('Filter: orderBy', function() {
    });


+    it('should support a custom comparator', function() {


I feel that the test is a bit too clever with "ownerD" being the nerd in question. ;)

I would go as far as calling it "nerdy" 😛

I added a bunch of other tests, so it's not so necessary any more - I just kept it around because I liked it. I'm fine removing it if you think it's too confusing.

Narretz · 2016-04-21T20:03:12Z

What if you want to have a custom comparator and the option to easily reverse the order?

With this change the 3rd argument is interpreted as a comparator function, used to compare the values returned by the predicates. Leaving it empty falls back to the default comparator and a value of `false` is a special case that also falls back to the default comparator but reverses the order. Thus the new implementation is backwards compatible. This commit also expands the documentation to cover the algorithm used to sort elements and adds a few more unit and e2e tests (unrelated to the change). Helps with angular#12572 (maybe this is as close as we want to get). Fixes angular#13238 Fixes angular#14455 Closes angular#5123 Closes angular#8112 Closes angular#10368

gkalpak · 2016-04-22T00:07:34Z

This is now ready for review 😄

I updated the docs and added more tests. Especially wrt to how we sort things (beyond the: "numbers numerically, strings alphabetically"). I believe that if people are to rely on some ordering algorithm, they have the right to know what that is.

Narretz · 2016-04-22T08:11:00Z

src/ng/filter/orderBy.js

+ * order (expression is set to `'-age'`). The `comparator` is not set, which means it defaults to
+ * the built-in comparator.
+ *
+   <example module="orderByExample1">


Can you name this and the other examples (name="desriptiveName"). That makes them easier to find in the build folder.

Narretz · 2016-04-22T09:58:35Z

Can you also mention in the docs that orderBy can only order alphabetically by latin characters? Actually, which characters are supported anyway?

gkalpak · 2016-05-26T20:00:45Z

@Narretz, I have addressed your comments. PTAL

Regarding the "supported" characters:
It uses the default < comparison (so it's not latin characters only). It is just that it doesn't do locale-sensitive comparison (and in some locales it makes a difference). The "locale-insensitive-ness" of the default comparator is already mentioned - other than that all characters are supported (in a sense).

Narretz · 2016-05-27T15:08:17Z

Thanks for the update.
I remember we talked about how to keep the API backwards compatible in a sane way. Did you change anything about this since?

gkalpak · 2016-05-27T23:00:58Z

This is already backwards compatible - but you are right we did talk about something:
Making it easy to have a custom comparator and still be able to specify ascending/descending order.

We settled for orderBy(collection[, expression[, reverse[, comparator]]]). I'll make the changes.

- Decouple `reverse` from `comparator` arguments.

gkalpak · 2016-05-30T18:00:08Z

@Narretz, I moved comparator as a 4th argument. Now you can specify reverse and comparator independently. PTAL

Narretz · 2016-06-06T08:32:38Z

src/ng/filter/orderBy.js

- * @returns {Array} Sorted copy of the source array.
+ * @param {boolean=} reverse - If `true`, reverse the sorting order.
+ * @param {(Function)=} comparator - The comparator function used to determine the relative order of
+ *    value pairs. If omitted, a built-in comparator will be used.


Is there more than one built-in comparator? Or should it be "the default comparator"?

There's only one. Changed it to the.

Narretz · 2016-06-06T09:11:42Z

I left a few smaller comments where I think the docs could be clarified. I've also opened a PR in your repo that adds descriptions ot the examples
Otherwise LGTM. Maybe @petebacondarwin can give it another look, too.

petebacondarwin · 2016-06-06T10:27:43Z

test/ng/filter/orderBySpec.js

+
+
+    describe('(reversing order)', function() {
+      it('should not reverse collection if `comparator` param is falsy',


comparator param -> reverse param

petebacondarwin · 2016-06-06T10:29:20Z

Apart from the couple of nitpicks in docs and tests, this LGTM.
Don't forget to fix up the commit message.

- Fix typos and wording.

Add an optional, 4th argument (`comparator`) for specifying a custom comparator function, used to compare the values returned by the predicates. Omitting the argument, falls back to the default, built-in comparator. The 3rd argument (`reverse`) can still be used for controlling the sorting order (i.e. ascending/descending). Additionally, the documentation has been expanded to cover the algorithm used by the built-in comparator and a few more unit and e2e tests (unrelated to the change) have been added. Helps with #12572 (maybe this is as close as we want to get). Fixes #13238 Fixes #14455 Closes #5123 Closes #8112 Closes #10368 Closes #14468

gkalpak · 2016-06-06T21:46:26Z

Fixed and merged! Thx @Narretz and @petebacondarwin for the reviews 👍

googlebot added the cla: yes label Apr 19, 2016

gkalpak added type: feature frequency: low needs: review severity: inconvenient component: filters labels Apr 19, 2016

gkalpak added this to the 1.6.x milestone Apr 19, 2016

Narretz reviewed Apr 21, 2016
View reviewed changes

gkalpak force-pushed the feat-orderBy-support-custom-comparator branch from c2cf7fe to 60dbf04 Compare April 21, 2016 23:55

gkalpak force-pushed the feat-orderBy-support-custom-comparator branch from 60dbf04 to 2d7bb9d Compare April 21, 2016 23:58

gkalpak changed the title ~~WIP - feat(orderBy): add support for custom comparators~~ feat(orderBy): add support for custom comparators Apr 22, 2016

Narretz reviewed Apr 22, 2016
View reviewed changes

fixup 1

ef6d0be

fixup 2

a35fd7a

- Decouple `reverse` from `comparator` arguments.

Narretz reviewed Jun 6, 2016
View reviewed changes

petebacondarwin reviewed Jun 6, 2016
View reviewed changes

fixup 3

f983563

- Fix typos and wording.

gkalpak closed this in 48c8b23 Jun 6, 2016

gkalpak mentioned this pull request Jun 6, 2016

feat(orderBy): Support natural sort #12572

Closed

gkalpak deleted the feat-orderBy-support-custom-comparator branch June 7, 2016 06:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(orderBy): add support for custom comparators #14468

feat(orderBy): add support for custom comparators #14468

gkalpak commented Apr 19, 2016 •

edited

Loading

Narretz Apr 21, 2016

gkalpak Apr 22, 2016

Narretz commented Apr 21, 2016

gkalpak commented Apr 22, 2016 •

edited

Loading

Narretz Apr 22, 2016

Narretz commented Apr 22, 2016

gkalpak commented May 26, 2016

Narretz commented May 27, 2016

gkalpak commented May 27, 2016

gkalpak commented May 30, 2016

Narretz Jun 6, 2016

gkalpak Jun 6, 2016

Narretz commented Jun 6, 2016

petebacondarwin Jun 6, 2016

petebacondarwin commented Jun 6, 2016

gkalpak commented Jun 6, 2016

		@@ -157,6 +183,38 @@ describe('Filter: orderBy', function() {
		});


		it('should support a custom comparator', function() {



		describe('(reversing order)', function() {
		it('should not reverse collection if `comparator` param is falsy',

feat(orderBy): add support for custom comparators #14468

feat(orderBy): add support for custom comparators #14468

Conversation

gkalpak commented Apr 19, 2016 • edited Loading

Narretz Apr 21, 2016

Choose a reason for hiding this comment

gkalpak Apr 22, 2016

Choose a reason for hiding this comment

Narretz commented Apr 21, 2016

gkalpak commented Apr 22, 2016 • edited Loading

Narretz Apr 22, 2016

Choose a reason for hiding this comment

Narretz commented Apr 22, 2016

gkalpak commented May 26, 2016

Narretz commented May 27, 2016

gkalpak commented May 27, 2016

gkalpak commented May 30, 2016

Narretz Jun 6, 2016

Choose a reason for hiding this comment

gkalpak Jun 6, 2016

Choose a reason for hiding this comment

Narretz commented Jun 6, 2016

petebacondarwin Jun 6, 2016

Choose a reason for hiding this comment

petebacondarwin commented Jun 6, 2016

gkalpak commented Jun 6, 2016

gkalpak commented Apr 19, 2016 •

edited

Loading

gkalpak commented Apr 22, 2016 •

edited

Loading