Add Order->Ordering implicit conversion to implicits, instances #1670
Conversation
| object Order extends OrderFunctions[Order] { | ||
| trait OrderToOrderingConversion { | ||
| /** | ||
| * Implicitly convert a `Order[A]` to a `scala.math.Ordering[A]` |
There was a problem hiding this comment.
This is not an implicit conversion which is something else. Can we say something like "provide an Ordering instance equivalent to the Order".
It does convert, and so I know I'm being pedantic, but I really don't like implicit conversions except for method enrichment, and in any case an implicit conversion has exactly one non-implicit parameter.
There was a problem hiding this comment.
I agree, but I changed this comment already. I am not seeing how it showed up here 😛
There was a problem hiding this comment.
Ah sorry. Forgot to push. Is the edited one alright?
edmundnoble
force-pushed
the
order-ordering-conversion
branch
from
b7aa10c to
015e5f2
Compare
| ev.toOrdering | ||
| } | ||
|
|
||
| object Order extends OrderFunctions[Order] with OrderToOrderingConversion { |
There was a problem hiding this comment.
I haven't researched this, but I am wondering if we place the method directly in the object instead of a trait, will that help binary comp going forward (on scala 2.11.x)?
There was a problem hiding this comment.
Is there any benefit to having implicit final def catsKernelOrderingForOrder[A](implicit ev: Order[A]): Ordering[A] inside the Order companion object? I don't think that scala will look here when it is searching for an Ordering instance. I could see a minor benefit of having a non-implicit version of this method inside the companion object. As it stands it seems like it's just a liability as I would expect you to get ambiguous implicits if you've imported both cats.implicits._ and cats.kernel.Order._.
There was a problem hiding this comment.
Now I don't know why I thought that would be a valid point to make. It has to be a trait for cats.implicits._. Sorry about the noise.
|
for those following at home, the mima check failed on kernel. |
|
I know that this isn't binary compatible. Is code duplication an acceptable fix? |
|
@edmundnoble code duplication is an acceptable fix (i.e. they don't need to share the trait). |
edmundnoble
force-pushed
the
order-ordering-conversion
branch
from
015e5f2 to
a3b611b
Compare
Codecov Report
@@ Coverage Diff @@
## master #1670 +/- ##
==========================================
+ Coverage 94.17% 94.17% +<.01%
==========================================
Files 256 256
Lines 4207 4208 +1
Branches 93 93
==========================================
+ Hits 3962 3963 +1
Misses 245 245
Continue to review full report at Codecov.
|
edmundnoble
force-pushed
the
order-ordering-conversion
branch
from
a3b611b to
ed8c873
Compare
|
Binary compatible, now. |
|
I wonder if this is going to create ambiguous implicits. Can we have a test where we do |
|
Ambiguity test added; it passes, I'm fairly sure the reason is that the |
edmundnoble
force-pushed
the
order-ordering-conversion
branch
from
69fab8c to
87c9bda
Compare
edmundnoble
force-pushed
the
order-ordering-conversion
branch
from
3fe1cad to
5179bb3
Compare
|
@johnynek Any more feedback? Just rebased on master and squashed. |
|
👍 |
|
Merged with two sign-offs. |
Does... anyone know where I would put a test? If it's worth testing? Also, kernel is open for changes for 1.0, right?