-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Don't require Order
for the value on Cogen
for SortedMap
and NonEmptyMap
#4296
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -130,7 +130,7 @@ object arbitrary extends ArbitraryInstances0 with ScalaVersionSpecific.Arbitrary | |
a <- A.arbitrary | ||
} yield NonEmptyMap((k, a), fa)) | ||
|
||
implicit def cogenNonEmptyMap[K: Order: Cogen, A: Order: Cogen]: Cogen[NonEmptyMap[K, A]] = | ||
implicit def cogenNonEmptyMap[K: Order: Cogen, A: Cogen]: Cogen[NonEmptyMap[K, A]] = | ||
Cogen[SortedMap[K, A]].contramap(_.toSortedMap) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For binary compatibility we have to keep the old signature as well, like this: @deprecated("Preserved for bincompat", "2.9.0")
def cogenNonEmptyMap[K, A](kOrder: Order[K], kCogen: Cogen[K], aOrder: Order[A], aCogen: Coge[A]): Cogen[NonEmptyMap[K, A]] There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do you mean copy the old signature and make it non implicit? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yes, exactly. |
||
|
||
implicit def catsLawsArbitraryForEitherT[F[_], A, B](implicit | ||
|
@@ -286,9 +286,8 @@ object arbitrary extends ArbitraryInstances0 with ScalaVersionSpecific.Arbitrary | |
implicit def catsLawsArbitraryForSortedMap[K: Arbitrary: Order, V: Arbitrary]: Arbitrary[SortedMap[K, V]] = | ||
Arbitrary(getArbitrary[Map[K, V]].map(s => SortedMap.empty[K, V](implicitly[Order[K]].toOrdering) ++ s)) | ||
|
||
implicit def catsLawsCogenForSortedMap[K: Order: Cogen, V: Order: Cogen]: Cogen[SortedMap[K, V]] = { | ||
implicit def catsLawsCogenForSortedMap[K: Order: Cogen, V: Cogen]: Cogen[SortedMap[K, V]] = { | ||
implicit val orderingK: Ordering[K] = Order[K].toOrdering | ||
implicit val orderingV: Ordering[V] = Order[V].toOrdering | ||
|
||
implicitly[Cogen[Map[K, V]]].contramap(_.toMap) | ||
} | ||
|
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.
I'm confused why we even need the
K: Order
here. ASortedMap
knows theOrdering[K]
.Doesn't this also work only with
K: Cogen, A: Cogen
?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.
I was confused about this as well :) Scalacheck implements the
Cogen[Map]
by transforming it to an ordered collection.https://github.com/typelevel/scalacheck/blob/7bb1a47af2313f7e7935d34cce255f7b7c0b1b31/core/shared/src/main/scala/org/scalacheck/Cogen.scala#L129-L130
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.
Good catch! As a side note: in fact, there are quite a lot of places in the Cats codebase where a constraint is demanded but not actually used. So this is why I think it would be great to have "unused" warnings enforced eventually.