OneAnd: add PartialOrder and Order instances
#4460
Conversation
| @@ -280,6 +289,19 @@ sealed abstract private[data] class OneAndLowPriority0_5 extends OneAndLowPriori | |||
| } | |||
|
|
|||
| sealed abstract private[data] class OneAndLowPriority0 extends OneAndLowPriority0_5 { | |||
| implicit def catsDataPartialOrderForOneAnd[A, F[_]](implicit | |||
There was a problem hiding this comment.
You can put this together with Order because it's a supertype.
So if F[x] <: G[x] it's ok to put them on the same level.
But if you were to add Hash then it has to be on a separate level from Order because Eq is common between Hash and Order but neither is a subtype of the other.
There was a problem hiding this comment.
Thank you for the clarification 👍 Personally, I would like to gather them in one place (and I was considering to, initially).
However, I found out that looks like almost everywhere in Cats Order and PartialOrder are placed on different levels. Perhaps just due to an oversight though (many oversights, actually). So I was uncertain about it and decided to follow the convention – I thought it was made for a reason. But if there's no objections, I would put it together indeed.
There was a problem hiding this comment.
Since those are package private traits we are free to reorder them 👍
There was a problem hiding this comment.
@joroKr21 , I tried to put Order and PartialOrder at one level. It works good indeed for Scala 2.13, but Scala 2.12 fails in multiple places with errors like this one:
[error] .../github/typelevel/cats/tests/shared/src/test/scala-2.12/cats/tests/NonEmptyStreamSuite.scala:44:42: ambiguous implicit values:
[error] both method catsDataPartialOrderForOneAnd in class OneAndInstances of type [A, F[_]](implicit A: cats.PartialOrder[A], implicit FA: cats.PartialOrder[F[A]])cats.PartialOrder[cats.data.OneAnd[F,A]]
[error] and method catsDataOrderForOneAnd in class OneAndInstances of type [A, F[_]](implicit A: cats.Order[A], implicit FA: cats.Order[F[A]])cats.Order[cats.data.OneAnd[F,A]]
[error] match expected type cats.kernel.Eq[cats.data.NonEmptyStream[Int]]
[error] checkAll("NonEmptyStream[Int]", EqTests[NonEmptyStream[Int]].eqv)
[error] ^
At least, I know now, why those instances are organized in such a way across the Cats :)
There was a problem hiding this comment.
Ahh damn, I think it's a bug when there are higher-kinded type parameters. Got it, thanks 👍
satorg
force-pushed
the
add-oneand-orders
branch
from
5eca13f to
2999e7b
Compare
satorg
force-pushed
the
add-oneand-orders
branch
from
2999e7b to
9dab5a5
Compare
| { | ||
| import Helpers.Eqed | ||
|
|
||
| checkAll("OneAnd[F, A]", EqTests[OneAnd[ListWrapper, Eqed]].eqv) |
There was a problem hiding this comment.
Were the Eq laws not already tested?
There was a problem hiding this comment.
I agree it may look superfluous (and probably it is). So if you don't like I am up to remove this check.
The motivation though was to give a try to the Eqed type that provides a "pure" Eq instance. So if somehow Eq is masked or overridden by Order or something and is not consistent with the laws, then it potentially could help to catch this. But I agree – it has a very little practical sense.
Also, I noticed that Eqed not in use in any other tests so decided to give it a try here.
There was a problem hiding this comment.
Oh, no, you misunderstood, I liked it!
I was just confirming that even though the Eq instance already existed, there were no existing law tests for it actually.
satorg
force-pushed
the
add-oneand-orders
branch
from
9dab5a5 to
9623b83
Compare
|
Thank you guys for looking into it, I appreciate your time. I would merge it if there are no objections. |
PartialOrderandOrderinstances toOneAnd.PartialOrder,Orderand already existingEqinstances according to their supposed resolution order.