Skip to content
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

Merged
merged 11 commits into from
Nov 22, 2017
Merged

Add Ior Monad Transformer #1977

merged 11 commits into from
Nov 22, 2017

Conversation

frroliveira
Copy link
Contributor

Initial work for #1943. Happy to add docs and more methods (that EitherT has), if this seems ok

@frroliveira frroliveira reopened this Oct 25, 2017
@codecov-io
Copy link

codecov-io commented Oct 25, 2017

Codecov Report

Merging #1977 into master will decrease coverage by 0.96%.
The diff coverage is 97.43%.

Impacted file tree graph

@@            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
Impacted Files Coverage Δ
...rc/main/scala/cats/laws/discipline/Arbitrary.scala 98.71% <100%> (+6.53%) ⬆️
core/src/main/scala/cats/data/IorT.scala 97.41% <97.41%> (ø)
free/src/main/scala/cats/free/Yoneda.scala 80% <0%> (-20%) ⬇️
free/src/main/scala/cats/free/Coyoneda.scala 84.61% <0%> (-7.06%) ⬇️
core/src/main/scala/cats/data/Tuple2K.scala 94.44% <0%> (-5.56%) ⬇️
core/src/main/scala/cats/data/Nested.scala 94.82% <0%> (-5.18%) ⬇️
core/src/main/scala/cats/instances/map.scala 90% <0%> (-4.29%) ⬇️
core/src/main/scala/cats/data/Func.scala 96.42% <0%> (-3.58%) ⬇️
core/src/main/scala/cats/data/EitherK.scala 97.82% <0%> (-2.18%) ⬇️
free/src/main/scala/cats/free/FreeT.scala 90.41% <0%> (-1.26%) ⬇️
... and 69 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9277979...adc87b7. Read the comment docs.

@kailuowang
Copy link
Contributor

@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 kailuowang added this to the 1.0.0 milestone Oct 27, 2017
@frroliveira
Copy link
Contributor Author

@kailuowang No problem. Thanks for letting me know

Copy link
Member

@LukaJCB LukaJCB left a 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] =
Copy link
Member

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]? :)

Copy link
Contributor Author

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] =
Copy link
Member

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.

Copy link
Contributor Author

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]]) {
Copy link
Member

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]

@frroliveira
Copy link
Contributor Author

Do you guys prefer docs to be merged with code or with separate PR?

@kailuowang
Copy link
Contributor

@frroliveira it would be great to include docs in this PR as well.

@frroliveira
Copy link
Contributor Author

Sorry it took me some time to get the docs.
Meanwhile, I have added an extra method condF. It would probably be useful in EitherT as well.

Regarding another PR that has been merged. Should I keep the order of the MonadError instances? Or switch them to be consistent with EitherT?

@frroliveira
Copy link
Contributor Author

frroliveira commented Nov 19, 2017

Ah, and I forgot. The apt package manager does not install the latest version of jekyll, so I updated the Contributing file

LukaJCB
LukaJCB previously approved these changes Nov 20, 2017
Copy link
Member

@LukaJCB LukaJCB left a 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] =
Copy link
Contributor

@kailuowang kailuowang Nov 20, 2017

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

LukaJCB
LukaJCB previously approved these changes Nov 21, 2017
* 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)
Copy link
Contributor

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sure, no problem

Copy link
Contributor

@kailuowang kailuowang left a 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!

@kailuowang kailuowang merged commit ab5433e into typelevel:master Nov 22, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants