-
Notifications
You must be signed in to change notification settings - Fork 3.5k
Add compare support to Enum.sort and add Enum.sort_reverse* #9426
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
Conversation
Before this PR, sorting dates, decimals, and other structs was somewhat awkward: Enum.sort(dates, &(Date.compare(&1, &2) != :lt)) # ascending Enum.sort(dates, &(Date.compare(&1, &2) != :gt)) # descending Sorting a user by dates, decimals, and other structs was equally verbose: Enum.sort_by(users, &(&1.birthday), &(Date.compare(&1, &2) != :lt)) # ascending Enum.sort_by(users, &(&1.birthday), &(Date.compare(&1, &2) != :gt)) # descending Even a general sort in reverse required passing &>=/2 as an argument, which is not clear in intent: Enum.sort(list, &>=/2) # descending This PR addresses this by adding two changes: 1. Sorting funtions now accept a module as sorter. The compare/2 function of said modules are then used for sorting. 2. `sort_reverse*` functions were added to complement the functionality above All of the examples above can be written as: Enum.sort(dates, Date) # ascending Enum.sort_reverse(dates, Date) # descending Enum.sort_by(users, &(&1.birthday), Date) # ascending Enum.sort_reverse_by(users, &(&1.birthday), Date) # descending Enum.sort_reverse(list) # descending The reason why we picked `sort_reverse` instead of `sort_descending` because a sorting funtion can still be given to `sort_reverse`, which means a descending function will return an ascending result (i.e. the reverse).
The doctests are enough to provide full overage but I will also add some specific unit tests to the new functionality in the next hour or so. In any case, this is good for feedback. |
Very interesting! I like this proposal; I think it is much clearer and cleaner than the earlier ideas that were floating around. 👍 |
@ericmj @josevalim there's a hiccup with Decimal in that we have: iex> Decimal.cmp(1, 2) # :lt | :eq | :gt
:lt
iex> Decimal.compare(1, 2) # Decimal<-1> | Decimal<0> | Decimal<1>
#Decimal<-1> Given compare/2 returning |
@wojtekmach my suggestion is to ship a Decimal version ASAP that deprecates the current use of compare. |
@josevalim Personally I do not like adding extra functions Maybe it would be better if we would consider adding For example: Enum.sort(list, Date, reverse: true)
Enum.sort_by(list, &(&1.birthday), Date, reverse: true)
# instead of
Enum.sort_reverse(list, Date)
Enum.sort_reverse_by(list, &(&1.birthday), Date) Maybe further in Enum.sort(list, reverse: true, sorter: Date)
Enum.sort_by(list, &(&1.birthday), reverse: true, sorter: Date)
# instead of
Enum.sort_reverse(list, Date)
Enum.sort_reverse_by(list, &(&1.birthday), Date) Of course this version is not backwards compatible, but in my opinion it looks just better.
defmodule Example do
def sample(params) do
list = get_list(params)
order = params["order"] || "asc"
Enum.sort_by(list, &(&1.birthday), Date, reverse: order != "asc")
end
# …
end
# instead of
defmodule Example do
def sample(params) do
list = get_list(params)
order = params["order"] || "asc"
name = if order == "asc", do: :sort_by, else: :sort_reverse_by
apply(Enum, name, [list, &(&1.birthday), Date, reverse: order != "asc"])
end
# or
def sample(params) do
order = params["order"] || "asc"
params |> get_list() |> do_sample(order)
end
defp do_sample(list, "asc"), do: Enum.sort_by(list, &(&1.birthday), Date)
defp do_sample(list, "desc"), do: Enum.sort_reverse_by(list, &(&1.birthday), Date)
# …
end |
I see the point regarding options but nothing in Enum accepts options and composition of Enum functions is done by concatenating names, so I don't want to introduce a new precedent. |
What about formalizing this as a Protocol? Something akin to Ruby’s Comperable? Then we can skip the option all together by implementing a new protocol, where Any falls back to something that returns the same values as Date.compare/2? Also, I take it the reason for sort_reverse being implemented is performance? If it’s not faster, why add these functions instead of having the user sort and then reverse their list? |
To clarify, it cannot be a protocol. A protocol includes dispatch, but that
is single dispatch, and compare requires multiple dispatch.
It can, however, be a behavior and we do plan to officialize it sooner
rather than later, in a future PR.
--
*José Valimwww.plataformatec.com.br
<http://www.plataformatec.com.br/>Founder and Director of R&D*
|
Oh, yeah. And I guess if we change Enum.sort/1 to automatically use the implementation of that behaviour that would be a breaking change, so I guess that’s not an option right now, either (even though it’s most likely the behaviour that newcomers to the language would expect). |
Correct. And we can’t even choose a default implementation of a behavior,
if you are comparing two different types, Float and Decimal, which behavior
do you pick? Hence the multiple dispatch challenge and why having a
behavior that you simply pass as argument.
--
*José Valimwww.plataformatec.com.br
<http://www.plataformatec.com.br/>Founder and Director of R&D*
|
Closing in favor of #9432. |
Before this PR, sorting dates, decimals, and other structs was
somewhat awkward:
Sorting a user by dates, decimals, and other structs was equally
verbose:
Even a general sort in reverse required passing &>=/2 as an argument,
which is not clear in intent:
This PR addresses this by adding two features:
Sorting funtions now accept a module as sorter. The compare/2
function of said modules are then used for sorting.
sort_reverse*
functions were added to complement thefunctionality above
All of the examples above can be written as:
The reason why we picked
sort_reverse
instead ofsort_descending
is because a sorting funtion can still be given to
sort_reverse
, whichmeans a descending function will return an ascending result (i.e. the
reverse).