summon ApplicativeErrorSyntax for F[_] instead of F[_, _]
#1046
Conversation
| implicit def applicativeErrorIdSyntax[E](e: E): ApplicativeErrorIdOps[E] = | ||
| new ApplicativeErrorIdOps(e) | ||
|
|
||
| implicit def applicativeErrorSyntax[F[_, _], E, A](fa: F[E, A])(implicit F: ApplicativeError[F[E, ?], E]): ApplicativeErrorOps[F[E, ?], E, A] = | ||
| implicit def applicativeErrorSyntax2[F[_, _], E, A](fa: F[E, A])(implicit F: ApplicativeError[F[E, ?], E]): ApplicativeErrorOps[F[E, ?], E, A] = |
There was a problem hiding this comment.
I’d rather replace this one with a variant using Unapply.
There was a problem hiding this comment.
To me Unapply is hard to read, I thought this variant does the same thing, so if you don't mind, could you educate what's the benefit of using Unapply over this variant?
There was a problem hiding this comment.
Unapply is a more general solution to the type inference problem that you are currently solving with this PR. But it will (hopefull soon!) become useless because SI-2712 is going to be fixed.
Once this will be the case, projects like cats will follow some migration path to get rid of the current workarounds. One possible migration path could be to move every Unapply-related methods to an external project that could be kept maintained just for compatibility with old Scala versions.
So, using Unapply clearly marks that the implicit definition will be useless once SI-2712 is fixed.
There was a problem hiding this comment.
@julienrf, being consistent is a good point although I would imagine you want to cross build this code, meaning, I am not sure you can to simply remove the Unapply usage in the code base.
@ceedubs mentioned in the chatroom that we don't want to over invest in Unapply post the SI-2712 fix. That being said, I am totally willing to turn this into Unapply, if that's the consensus.
Maintainers any thoughts?
There was a problem hiding this comment.
I am hopeful that Unapply's days are numbered. However, if we are going to add this sort of thing to help with inference (which is probably its own question now that the SI-2712 fix plugin exists), I think that we should reuse Unapply instead of less general workarounds like this one. Having said that, I briefly tried hooking up Unapply to this and was having trouble getting the syntax to be recognized for Xor - maybe someone else can do a better job than I did?
There was a problem hiding this comment.
@ceedubs do we want to do the originally proposed change, which is, just change it to F[_]?
There was a problem hiding this comment.
@kailuowang yeah, that sounds reasonable to me. Have you by any chance checked that works when using the SI-2712 fix?
@travisbrown do you have any strong opinions on this? I think you originally added the applicative error syntax.
There was a problem hiding this comment.
Just checked with the SI-2712 fix, it works fine (can find syntax for F[_, _] with only one F[_] variant.
Removing the existing F[_, _] variant, however, would mean we need to change existing syntax tests to use Option instead of Xor, right?
There was a problem hiding this comment.
Yes, or introduce a type alias.
There was a problem hiding this comment.
per @travisbrown at gitter chat, I am going to replace the F[_, _] with F[_] and change tests to use Option
Current coverage is 88.57%@@ master #1046 diff @@
==========================================
Files 214 214
Lines 2719 2720 +1
Methods 2655 2656 +1
Messages 0 0
Branches 59 59
==========================================
+ Hits 2407 2409 +2
+ Misses 312 311 -1
Partials 0 0
|
…t SI-2712 fix rule it all
kailuowang
changed the title
ApplicativeErrorSyntax for F[_]ApplicativeErrorSyntax for F[_] instead of F[_, _]
|
I don't have a strong opinion here, and I've never used the |
|
Seems reasonable to me. 👍 |
|
👍 |
|
Thanks @kailuowang, I think this could be really useful. |
No description provided.