Optimize Alternative (part 3): add prependK/appendK specializations for Cats NE wrappers#4055
Conversation
|
|
||
| implicit val catsDataInstancesForNonEmptyChain: SemigroupK[NonEmptyChain] | ||
| // Required for binary compatibility with v2.6.1. | ||
| def catsDataInstancesForNonEmptyChain: SemigroupK[NonEmptyChain] |
There was a problem hiding this comment.
Should we either deprecate these or make them private[data]?
There was a problem hiding this comment.
I was pretty sure that I tried to make it package private in the original PR and it didn't work – it broke binary compatibility for some Scala version (or at least Mima though it does). But I've just given it another try and don't see any errors from Mima neither for Scala 2.12 nor for Scala 2.13. I got confused a little bit :)
On the second thought, I would assume that deprecating it might be slightly better because making it package private potentially (although unlikely) breaks source compatibility – in case if someone references this field explicitly in their code. WDYT?
There was a problem hiding this comment.
Oh, @djspiewak just mentioned in another issue:
We generally don't consider implicits to be part of source compatibility, so IMO this is something we can (and should) do in 2.7.1
Good to know, thanks. I guess I can safely make it package private then (since it seemingly doesn't break BC).
There was a problem hiding this comment.
Turns out, private[data] works for catsDataInstancesForNonEmptyChain only. For three other similar obsolete former implicits in this PR it breaks the binary compatibility. So I annotated them as deprecated without changes in their visibility.
There was a problem hiding this comment.
Were the mima problems only for static methods? I don't know what the policy about static methods is for cats, since those are only used by Java.
d14532f to
c97d140
Compare
|
Looking pretty good, but we should wait until MiMa is fixed in #4063. |
c97d140 to
5d72851
Compare
|
Hi @armanbilge , just to confirm with you: is this PR still blocked by #4063 ? |
|
Ah, this one has been sitting for a while, sorry! IIRC this PR made some changes to type signatures that looked like they could cause the same bincompat issues that we had with the I would now say this PR is blocked by #4143 which will enable MiMa for Scala 3 and supersede #4063. I hope to work on that this weekend. |
|
@satorg I believe I owe you a review here, also do you mind fixing up the conflicts 😅 |
|
Yeah, I'm keeping eye on it.. I will be happy to resume working on this.. Just a lil'bit busy on my work project now – it is approaching a deadline so quite difficult to put aside time for anything else. |
5d72851 to
ade8ce3
Compare
|
@armanbilge here it is, finally :) |
|
That's great, thanks for all your work on this! I'm currently buried working on typelevel/cats-effect#3138 (comment) but I promise we will get this into 2.9.0. |
armanbilge
changed the title
Alternative (part 3): add prependK/appendK specializations for Cats NE wrappers
armanbilge
left a comment
armanbilge
left a comment
There was a problem hiding this comment.
This is quite straightforward actually :) LGTM.
The only issue was the version in deprecations should be 2.9.0.
ade8ce3 to
475eff7
Compare
|
Bumped up deprecation versions to "2.9.0". |
3 participants
Adds optimized specializations for the new
prependK/appendKmethods introduced in the initial PR (#4014).Previous PR:
Subsequent PR:
Alternative(part 4): addprependK/appendKspecializations for Cats monad transformers #4299