MTL is dead, long live MTL - #1751
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1751 +/- ##
==========================================
- Coverage 95.32% 94.81% -0.51%
==========================================
Files 265 241 -24
Lines 4301 4092 -209
Branches 96 97 +1
==========================================
- Hits 4100 3880 -220
- Misses 201 212 +11
Continue to review full report at Codecov.
|
|
Just a thought: in the proposed design, the |
|
I'm -1 on removing ApplicativeError. What is the motivation for this? |
|
yeah, and we could have a |
|
Wouldn't |
|
wait, can't we make an seems like you can, but I didn't write all the methods. Why do you think you can't? Anything concrete? |
|
okay, I guess you can't: case class ValidatedT[F[_], E, A](f: F[Validated[E, A]])
class ValidatedTApplicativeError[F[_]: Applicative, E] extends ApplicativeError[ValidatedT[F, E, ?]] {
def pure[A](a: A): ValidatedT(Applicative[F].pure(Validated.valid(a)))
def raiseError[A](e: E): ValidateT(Applicative[F].pure(Validated.invalid(e)))
def handleErrorWith[A](fa: ValidatedT[F, E, A])(f: E => ValidateT[F, E, A]): ValidatedT[F, E, A] =
// here we can't look inside F[Validate[E, A]] to see if have an error
// we need a monad on F to do this, it seems.
}
class ValidatedTApplicativeError[F[_]: Applicative, E: Semigroup] extends ApplicativeError[Lambda[T => Validated[E, F[T]]]] {
def pure[A](a: A): Validated.valid(Applicative[F].pure(a))
def ap[A, B](f: Validated[E, F[A => B]])(a: Validated[E, F[A]]) =
f.product(a).map { case (ff, fa) => ff(fa) }
def raiseError[A](e: E) = Validated.invalid(e)
def handleErrorWith[A](fa: Validated[E, F[A]])(f: E => Validated[E, F[A]]): Validated[E, F[A]] =
fa match {
case v@Validated.Valid(_) => v
case Validated.Invalid(e) => f(e)
}
}so, you can't hide the error inside the F. It may be more generally that you can compose |
|
It didn't come to my mind that the removal of |
|
You've hit the nail on the head here, basically. This is why cats-mtl splits it into two classes: @kailuowang I have already moved the methods to My motivation for removing It's not a large issue to keep |
|
To avoid bogging this PR down, I'm taking a vote here: 👍 to keep Edit: This vote will be over on Thursday at noon ET. Ties will be counted as "keep". Edit 2: Keep it is then. |
|
How quickly will a release of |
| } | ||
|
|
||
| private[data] sealed abstract class WriterTInstances10 { | ||
| implicit def catsDataFunctorForWriterT[F[_], L](implicit F: Functor[F]): Functor[WriterT[F, L, ?]] = |
There was a problem hiding this comment.
Not sure why this is needed. The Coflatmap instance should suffice, no?
|
If it's not big deal, I vote for keeping the As a related note, I don't have a strong argument for it but I start to feel that the error effect might be worth some special treatment. We did that for |
|
I don't particularly feel myself like errors deserve any special treatment, however I observe that MonadError does not add any laws to ApplicativeError and it is okay to special-case one MTL class and still avoid ambiguity. |
5154353 to
953f442
Compare
edmundnoble
changed the title
6e88960 to
bc4caa9
Compare
| override def M = implicitly | ||
| override def M1 = implicitly | ||
| def ap[A, B](fa: FreeT[S, M, A])(ff: FreeT[S, M, A => B]): FreeT[S, M, B] = fa.flatMap(a => ff.map(_(a))(M)) | ||
| override final def empty[A]: FreeT[S, M, A] = FreeT.liftT[S, M, A](M1.empty[A])(M) |
There was a problem hiding this comment.
Not needed if extend with FreeTMonoidK
| implicit val freeTSemigroupK: SemigroupK[FreeTOption] = FreeT.catsFreeCombineForFreeT[Option, Option] | ||
| checkAll("FreeT[Option, Option, Int]", SemigroupKTests[FreeTOption].semigroupK[Int]) | ||
| implicit val freeTSemigroupK: SemigroupK[FreeTOption] = FreeT.catsFreeSemigroupKForFreeT[Option, Option] | ||
| checkAll("FreeT[Option, Option, Int]", MonoidKTests[FreeTOption].monoidK[Int]) |
bc7600f to
dc869c3
Compare
|
@kailuowang Comments addressed, thanks. |
|
thanks @edmundnoble I haven't gone through all of it yet. Will continue tomorrow. |
60ff270 to
8abd354
Compare
|
|
||
|
|
||
| class ApplicativeTests extends CatsSuite { | ||
| class ApplicativeCheck extends CatsSuite { |
There was a problem hiding this comment.
Not sure why this rename? I noticed that ApplicativeErrorCheck is also using the Check suffix. But that looks like an outlier? The vast majority have the suffix Tests. There is also MonadErrorSuite, BitraverseTest, CategoryTest, ComposeTest, ContravariantTest, FoldableCheck, ReducableCheck, FunctorTest, MonadTest. If you leave this one alone, I can work on another PR to consolidate them.
There was a problem hiding this comment.
Will do, good point. I didn't know about MonadErrorSuite or the rest of the Check-style suites.
| nes.filter(_ % 2 == 0) should ===(nes.unwrap.filter(_ % 2 == 0)) | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
Just curious, was the arbitrary function in the original test causing problems?
There was a problem hiding this comment.
I don't quite know what you mean. The original test in this case is MonadCombineTests, which has been removed, but I did needlessly use _ % 2 == 0 instead of an arbitrary function; that's been corrected now.
There was a problem hiding this comment.
Sorry I didn't make it clear. "needlessly use _ % 2 == 0 instead of an arbitrary function" is exactly what I intended to point out.
There was a problem hiding this comment.
Ah makes sense :)
8abd354 to
6647523
Compare
kailuowang
left a comment
kailuowang
left a comment
There was a problem hiding this comment.
👍 thanks so much for this heroic effort.
|
👍 on this pull request. I do have a concern about |
There was a problem hiding this comment.
With the removal of all the MTL instances we can clean up some of the "instance hierarchy traits".
I could try to do this clean up in a follow up PR, if you are too busy working on cats-mtl itself ...
And thanks for all this work @edmundnoble !!
| override def map[A, B](fa: EitherT[F, L, A])(f: A => B): EitherT[F, L, B] = fa map f | ||
| } | ||
|
|
||
| private[data] trait EitherTApply[F[_], L] extends EitherTFunctor[F, L] with Apply[EitherT[F, L, ?]] { |
There was a problem hiding this comment.
This Apply instance (and the Applicative instance below) is inconsistent with the Monad instance for EitherT, see #1467 (and the linked discussions).
There was a problem hiding this comment.
I think we need to drop these instances before we merge, all my other comments are things which can be cleaned up.
There was a problem hiding this comment.
This is very interesting, and will likely lead to some corrections in cats-mtl as it stands.
| } | ||
|
|
||
| private[data] sealed abstract class KleisliInstances6 extends KleisliInstances7 { | ||
| implicit def catsDataApplicativeErrorForKleisli[F[_], E, A](implicit F0: ApplicativeError[F, E]): ApplicativeError[Kleisli[F, A, ?], E] = |
There was a problem hiding this comment.
This ApplicativeError instance and the Alternative instance below probably should come before the Applicative, Apply and Functor instances.
The Alternative instance should also come before the MonoidK and SemigroupK instances.
| with RWSTSemigroupK[F, E, S, L] with RWSTMonadTrans[E, S, L] { | ||
| private[data] sealed trait RWSTAlternative[F[_], E, S, L] | ||
| extends Alternative[ReaderWriterStateT[F, E, S, L, ?]] | ||
| with RWSTSemigroupK[F, E, S, L] { |
There was a problem hiding this comment.
We can add with RWSTFunctor[F, E, S, L].
| } | ||
|
|
||
| private[data] sealed trait RWSTInstances5 extends RWSTInstances6 { | ||
| implicit def catsDataMonadForRWST[F[_], E, S, L](implicit F0: Monad[F], L0: Monoid[L]): Monad[ReaderWriterStateT[F, E, S, L, ?]] = |
There was a problem hiding this comment.
We can move this Monad instance to RSTInstances, now that all the other MTL MonadX instances are gone.
| } | ||
| ) | ||
|
|
||
| override def map[A, B](fa: StateT[F, S, A])(f: A => B): StateT[F, S, B] = fa.map(f)(F) |
There was a problem hiding this comment.
If we add override in StateTFunctor we can extend the Alternative and Monad instances, we don't have to override map explicitly.
There was a problem hiding this comment.
We can drop this line now that you've added with StateTFunctor.
| private[data] sealed abstract class WriterTInstances2 extends WriterTInstances3 { | ||
| implicit def catsDataMonadWriterForWriterT[F[_], L](implicit F: Monad[F], L: Monoid[L]): MonadWriter[WriterT[F, L, ?], L] = | ||
| new WriterTMonadWriter[F, L] { | ||
| implicit def catsDataMonadForWriterT[F[_], L](implicit F: Monad[F], L: Monoid[L]): Monad[WriterT[F, L, ?]] = |
There was a problem hiding this comment.
This can be moved to WriterTInstances0, which means that the Alternative, Applicative, FlatMap, ... instances can be moved up as well.
| } | ||
|
|
||
| private[free] sealed trait FreeTInstances2 extends FreeTInstances3 { | ||
| sealed trait FreeTInstances extends FreeTInstances0 { |
There was a problem hiding this comment.
Should we leave this as private[free] ?
| private[free] sealed trait FreeTInstances2 extends FreeTInstances3 { | ||
| implicit def catsFreeAlternativeForFreeT[S[_], M[_]: Alternative: Monad]: Alternative[FreeT[S, M, ?]] = | ||
| new Alternative[FreeT[S, M, ?]] with FreeTMonad[S, M] with FreeTMonoidK[S, M] { | ||
| override def M = implicitly[Alternative[M]] |
There was a problem hiding this comment.
I think we prefer Alternative[M] over implicitly[Alternative[M]].
| * `Yoneda[F, _]` is a functor for any `F`. | ||
| */ | ||
| implicit def catsFreeFunctorForYoneda[F[_]]: Functor[Yoneda[F, ?]] = | ||
| implicit def catsCofreeFunctorForYoneda[F[_]]: Functor[Yoneda[F, ?]] = |
There was a problem hiding this comment.
I think the Free here is the package (like we have catsDataFunctorForKleisli.
| { | ||
| // Invariant composition | ||
| implicit val instance = ListWrapper.invariant | ||
| implicit val instance = Nested.catsDataInvariantForNested(ListWrapper.invariant, ListWrapper.invariant) |
There was a problem hiding this comment.
Is there a reason that we need to create these instances for Nested (here and below) explicitly?
There was a problem hiding this comment.
I'll check again, I had a coverage drop which was fixed by this but I think there was an underlying problem I fixed.
aec604d to
0359f43
Compare
|
All feedback should be addressed. |
| implicit val L: Monoid[ListWrapper[Int]] = ListWrapper.monoid[Int] | ||
| implicit val appErr = WriterT.catsDataApplicativeErrorForWriterT[Validated[String, ?], ListWrapper[Int], String] | ||
| implicit val appErr = WriterT.catsDataApplicativeForWriterT[Validated[String, ?], ListWrapper[Int]] | ||
| implicit val iso = CartesianTests.Isomorphisms.invariant[WriterT[Validated[String, ?], ListWrapper[Int], ?]] |
There was a problem hiding this comment.
Minor, should we call this app instead of appErr ?
| new FreeTCombine[S, M] { | ||
| override def M = implicitly | ||
| override def M1 = implicitly | ||
| def ap[A, B](fa: FreeT[S, M, A])(ff: FreeT[S, M, A => B]): FreeT[S, M, B] = fa.flatMap(a => ff.map(_(a))(M)) |
There was a problem hiding this comment.
I don't think we need this, right?
With override this should give a compiler error because Apply#ap has the parameters in the other order (first ff then fa)
def ap[A, B](ff: F[A => B])(fa: F[A]): F[B]| } | ||
| ) | ||
|
|
||
| override def map[A, B](fa: StateT[F, S, A])(f: A => B): StateT[F, S, B] = fa.map(f)(F) |
There was a problem hiding this comment.
We can drop this line now that you've added with StateTFunctor.
0359f43 to
61dbe4e
Compare
| /** Converts to `F[A]` given that `F` is a functor */ | ||
| final def run(implicit F: Functor[F]): F[A] = F.map(fi)(k) | ||
|
|
||
| /** Converts to `F[A]` given that `F` is a functor */ |
There was a problem hiding this comment.
"Converts to G[A] given that G is a functor".
43d29b2 to
1e219f2
Compare
|
Merging with two approvals. |
|
🍻💕 |
10 participants
This PR removes the types ApplicativeError, FunctorFilter, MonadCombine, MonadFilter, MonadReader, MonadState, MonadTrans, MonadWriter, TraverseFilter.
It also replaces MonadCombine's operations with equivalents on Alternative (no laws for them anyway), moves methods on ApplicativeError to MonadError; all other functionality will be moved to cats-mtl if it is already not present.
Sorry about the length. There were some missing Alternative instances I had to add as well to make users of MonadCombine less unhappy about the change.