-
-
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
Add Ior Monad Transformer #1977
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1977 +/- ##
==========================================
- Coverage 96.21% 95.25% -0.97%
==========================================
Files 272 305 +33
Lines 4627 5179 +552
Branches 115 129 +14
==========================================
+ Hits 4452 4933 +481
- Misses 175 246 +71
Continue to review full report at Codecov.
|
@frroliveira thanks very much for this PR. To give you a heads up, at the moment we are focusing on pushing across the RC1 finish line. This PR since is completely backward compatible, although useful, is not the highest on the priority list right now. But we will get back to it later. |
@kailuowang No problem. Thanks for letting me know |
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.
Added a couple of minor comments, overall this looks really really good to me, so thank you very much! :)
} | ||
}) | ||
|
||
def flatMapF[AA >: A, D](f: B => F[Ior[AA, D]])(implicit F: Monad[F], AA: Semigroup[AA]): IorT[F, AA, D] = |
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.
Couldn't this be just F: FlatMap[F]
? :)
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.
Current implementation needs it to call flatMap
. I couldn't find a way to use it here, however collectRight
can be changed to F: FlatMap[F]
def flatMapF[AA >: A, D](f: B => F[Ior[AA, D]])(implicit F: Monad[F], AA: Semigroup[AA]): IorT[F, AA, D] = | ||
flatMap(f andThen IorT.apply) | ||
|
||
def subflatMap[AA >: A, D](f: B => Ior[AA, D])(implicit F: Monad[F], AA: Semigroup[AA]): IorT[F, AA, D] = |
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.
Same here, FlatMap
should be enough I think.
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.
This one can actually be Functor
import cats.syntax.either._ | ||
import cats.syntax.option._ | ||
|
||
final case class IorT[F[_], A, B](value: F[Ior[A, B]]) { |
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.
Can we also add a mapK
method, that applies a FunctionK
to the underlying F
? I.e.
def mapK[G](f: F ~> G): IorT[G, A, B]
Do you guys prefer docs to be merged with code or with separate PR? |
@frroliveira it would be great to include docs in this PR as well. |
Sorry it took me some time to get the docs. Regarding another PR that has been merged. Should I keep the order of the MonadError instances? Or switch them to be consistent with |
Ah, and I forgot. The apt package manager does not install the latest version of jekyll, so I updated the Contributing file |
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.
This is really really good stuff, thanks!
implicit def catsDataFoldableForIorT[F[_], A](implicit F: Foldable[F]): Foldable[IorT[F, A, ?]] = | ||
new IorTFoldable[F, A] { val F0: Foldable[F] = F } | ||
|
||
implicit def catsDataMonadErrorFForIorT[F[_], A, E](implicit FE: MonadError[F, E], A: Semigroup[A]): MonadError[IorT[F, A, ?], E] = |
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.
+1 on switching the order of these two MonadError instances to be consistent with EitherT
* res1: cats.data.IorT[Option,Nothing,Int] = IorT(None) | ||
* }}} | ||
*/ | ||
final def liftF[F[_], A, B](fb: F[B])(implicit F: Applicative[F]): IorT[F, A, B] = right(fb) |
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.
do you mind also add liftK
like this one https://github.com/SystemFw/cats/blob/4830e326acab64223fa642a2b39111665e590763/core/src/main/scala/cats/data/EitherT.scala#L365 ? If you don't have the time it's fine too, I can add one in a follow up PR.
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.
sure, no problem
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.
What a great contribution! Thanks!
Initial work for #1943. Happy to add docs and more methods (that EitherT has), if this seems ok