Allow 2-arity Array.sort-style comparators to be passed into sortBy functions

[suprememoocow]

Hi, firstly thanks for such a great library. I really love it.

I would like to provide Array.sort-style 2-arity comparator functions to sortBy functions as this method of comparing elements allows the greatest amount of flexibility (for example, reverse sorting of string elements, sorting of different types elements in the same array, or multi-level sorting)

By 2-arity comparator functions, I mean ones which take 2 arguments and allow the implementation to return 0, -1 or +1 depending on the order, for example:

function compare(a, b) {
  if (a is less than b by some ordering criterion)
     return -1;
  if (a is greater than b by the ordering criterion)
     return 1;
  // a must be equal to b
  return 0;
}

Backbone and some other libraries check the length of the comparator function in order to decide whether to use simple 1-arity function or comparator-style 2-arity function.

Let me know what you think: I'm happy to have a crack at doing the work via a pull-request, but would like your feedback on the implementation first.

[Bartvds]

+1 for 2-arity sort support (no preference for new method or overloaded).

Currently I go through a toArray() and then back to Lazy, feels odd.

[dtao]

Haha, I'm going to implement this just so that @Bartvds doesn't do that anymore! ;)

[dtao]

OK, this is implemented and will be in 0.3.3.

[dtao]

I apologize @suprememoocow for sort of jumping the gun. Re-reading your original message I see you were (maybe?) interested in doing the implementation yourself. Well, feel free to tweak if you see any issues w/ my version! (Past 3 commits or so covers it.)

I definitely love contributions and would like more, so I'll try to be more receptive in the future to someone expressing interest in submitting a PR.

[Bartvds]

wow @dtao, so quick 👍

[jdalton]

Relying on func.length is shaky and causes problems with methods like Function#bind as some shims don't set the bound functions .length. I've seen API in things like express get mucked up over libs bind/partial functionality because they didn't set a .length either. Backbone has also had problems with this recently and amended their docs to warn against the gotchas.

[dtao]

@jdalton I'm aware of (at least some of) the issues w/ using length. My inclination is to stick with something like the current implementation---because it should cover most cases---and simply document the edge cases such as Function#bind.

The obvious alternative I can think of would be to make a separate method, e.g. sortWith, to deal with functions that do comparisons and keep sortBy for functions that return values. Is that what you'd suggest? What does Lo-Dash do?

[jdalton]

The obvious alternative I can think of would be to make a separate method, e.g. sortWith, to deal with functions that do comparisons and keep sortBy for functions that return values. Is that what you'd suggest?

It can be handled with an options argument as well (more explicit).

What does Lo-Dash do?

We don't handle comparison functions with two arguments. I believe it was originally based on Ruby's sort_by

[suprememoocow]

Wow, that was a super quick response! @dtao, no problem - I haven't implemented anything yet. Was just being a considerate githubber :) Sounds like using a sortWith method is the best way to go.

[dtao]

Yeah, I'm also considering maybe just going with sort, actually. This would imply parity with Array.prototype.sort, which is in fact accurate (and informative).

[jdalton]

Yeah, I'm also considering maybe just going with sort, actually. This would imply parity with Array.prototype.sort, which is in fact accurate (and informative).

👍 for sort. Bonus is to ensure the sort is stable too.

[suprememoocow]

sort does make a lot of sense