-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
Base.Ordering: do not ever discard
order
parameter (#34719)
Prior to this commit, the `order` parameter was discarded if either `by` or `order` was passed. However, there is a sane interpretation for most of these combinations, and we should use that interpretation for least surprise. The one case that doesn't have an obvious interpretation is when a non-trivial `order` is passed together with an `lt` function; in that case it is better to error out rather than let it pass silently.
- Loading branch information
Showing
5 changed files
with
80 additions
and
13 deletions.
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,38 @@ | ||
using Test | ||
|
||
import Base.Order: Forward, Reverse | ||
|
||
# every argument can flip the integer order by passing the right value. Here, | ||
# we enumerate a few of these combinations and check that all these flips | ||
# compound so that in total we either have an increasing or decreasing sort. | ||
for (s1, rev) in enumerate([true, false]) | ||
for (s2, lt) in enumerate([>, <, (a, b) -> a - b > 0, (a, b) -> a - b < 0]) | ||
for (s3, by) in enumerate([-, +]) | ||
for (s4, order) in enumerate([Reverse, Forward]) | ||
if iseven(s1 + s2 + s3 + s4) | ||
target = [1, 2, 3] | ||
else | ||
target = [3, 2, 1] | ||
end | ||
@test target == sort([2, 3, 1], rev=rev, lt=lt, by=by, order=order) | ||
end | ||
end | ||
end | ||
end | ||
|
||
@test [1 => 3, 2 => 5, 3 => 1] == | ||
sort([1 => 3, 2 => 5, 3 => 1]) == | ||
sort([1 => 3, 2 => 5, 3 => 1], by=first) == | ||
sort([1 => 3, 2 => 5, 3 => 1], rev=true, order=Reverse) == | ||
sort([1 => 3, 2 => 5, 3 => 1], lt= >, order=Reverse) | ||
|
||
@test [3 => 1, 1 => 3, 2 => 5] == | ||
sort([1 => 3, 2 => 5, 3 => 1], by=last) == | ||
sort([1 => 3, 2 => 5, 3 => 1], by=last, rev=true, order=Reverse) == | ||
sort([1 => 3, 2 => 5, 3 => 1], by=last, lt= >, order=Reverse) | ||
|
||
|
||
struct SomeOtherOrder <: Base.Order.Ordering end | ||
|
||
@test_throws ErrorException sort([1, 2, 3], lt=(a, b) -> a - b < 0, order=SomeOtherOrder()) | ||
|
8eb0f9f
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Executing the daily package evaluation, I will reply here when finished:
@nanosoldier
runtests(ALL, isdaily = true)
8eb0f9f
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Something went wrong when running your job:
cc @maleadt
8eb0f9f
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Executing the daily benchmark build, I will reply here when finished:
@nanosoldier
runbenchmarks(ALL, isdaily = true)
8eb0f9f
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Your benchmark job has completed - possible performance regressions were detected. A full report can be found here. cc @ararslan