Don't require Order for the value on Cogen for SortedMap and NonEmptyMap
#4296
Conversation
| 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.
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.
Do you mean copy the old signature and make it non implicit?
armanbilge
changed the title
Order for the value on Cogen for SortedMap and NonEmptyMap
|
Added delegation as requested |
| cogenNonEmptyMap[K, A] | ||
| } | ||
|
|
||
| implicit def cogenNonEmptyMap[K: Order: Cogen, A: Cogen]: Cogen[NonEmptyMap[K, A]] = |
There was a problem hiding this comment.
I'm confused why we even need the K: Order here. A SortedMap knows the Ordering[K].
Doesn't this also work only with K: Cogen, A: Cogen?
There was a problem hiding this comment.
I was confused about this as well :) Scalacheck implements the Cogen[Map] by transforming it to an ordered collection.
There was a problem hiding this comment.
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.
|
@satorg did you want to re-review this? |
|
Oh, I thought that Oscar asked a question and we're awaiting an answer from @cquiroz ... |
|
Yes, I think this one is ready for merge :) |
It seems we don't need to require an
Orderinstance forCogenofSortedMapandNonEmptyMap. We only need it for the keysFixes #4295