Extend Functor implementation in the Traverse instance of EitherT, OptionT and Tuple2K #2191

barambani · 2018-03-12T23:12:53Z

This is about using the Functor instance in Traverse for the monad transformers EitherT and OptionT and in Tuple2K so to use its map implementation. This follows @peterneyens 's comment #2182 (comment) related to WriterT.

traverse for monad transformer

kailuowang · 2018-03-13T01:09:03Z

this breaks binary compatibility. unfortunately these traits were not sealed. We might need to extend the functor trait at the instance creation site, rather than the traits.

barambani · 2018-03-13T01:16:28Z

Thanks for the pointer, I was trying funny things. I will keep trying that way and see if I can make it keep the binary compatibility. Otherwise I will close it.

codecov-io · 2018-03-13T02:52:14Z

Codecov Report

Merging #2191 into master will increase coverage by 0.22%.
The diff coverage is 89.47%.

@@            Coverage Diff             @@
##           master    #2191      +/-   ##
==========================================
+ Coverage   94.73%   94.95%   +0.22%     
==========================================
  Files         330      333       +3     
  Lines        5582     5787     +205     
  Branches      209      222      +13     
==========================================
+ Hits         5288     5495     +207     
+ Misses        294      292       -2

Impacted Files	Coverage Δ
core/src/main/scala/cats/data/Kleisli.scala	`96.9% <ø> (-0.99%)`	⬇️
...in/scala/cats/data/IndexedReaderWriterStateT.scala	`99.04% <100%> (-0.02%)`	⬇️
core/src/main/scala/cats/data/EitherK.scala	`93.47% <100%> (-4.35%)`	⬇️
core/src/main/scala/cats/data/OptionT.scala	`97.46% <100%> (ø)`	⬆️
core/src/main/scala/cats/data/Nested.scala	`94.28% <100%> (-1.43%)`	⬇️
core/src/main/scala/cats/data/WriterT.scala	`91.37% <100%> (ø)`	⬆️
core/src/main/scala/cats/data/IorT.scala	`97.76% <100%> (ø)`	⬆️
core/src/main/scala/cats/data/EitherT.scala	`97.65% <66.66%> (+0.05%)`	⬆️
core/src/main/scala/cats/data/Tuple2K.scala	`90.69% <83.33%> (-4.66%)`	⬇️
... and 20 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 6b01eda...a178ec5. Read the comment docs.

peterneyens · 2018-03-13T08:48:22Z

Thanks for the follow up.

I think we can do something similar (if you are interested) for :

EitherT: Bitraverse we can override bimap.
Tuple2K : ContravariantMonoidal extend Contravariant (can drop its contamap definition).
RWST: Alternative extend SemigroupK
IorT: Traverse extends Functor

I might have missed some other opportunities.

barambani · 2018-03-13T11:19:32Z

Sure, I'm happy to cover as much as I can. I will give a look. Thanks for the great support

the code review

barambani · 2018-03-14T18:46:12Z

core/src/main/scala/cats/data/WriterT.scala


  override def unit: WriterT[F, L, Unit] = WriterT(F0.trivial[(L, Unit)])

  override def contramap[A, B](fa: WriterT[F, L, A])(f: B => A): WriterT[F, L, B] =
-    WriterT(F0.contramap(fa.run)((d: (L, B)) => (d._1, f(d._2))))
+    super[WriterTContravariant].contramap(fa)(f)


I couldn't find a way to remove this. If I do it I get this

[error] * in current version, classes mixing cats.data.WriterTContravariantMonoidal needs to update body of method contramap(cats.data.WriterT,scala.Function1)cats.data.WriterT [error] filter with: ProblemFilters.exclude[UpdateForwarderBodyProblem]("cats.data.WriterTContravariantMonoidal.contramap")

from Mima. @kailuowang , @peterneyens can you think to anything I can try ? Thanks a lot

barambani · 2018-03-14T18:53:28Z

There is still a problem with IndexedReaderWriterStateT as most of the derivations are inside sealed abstract class so I cannot compose with mixin. An option could be change them to trait (even though I'm not sure if it break the compatibility), but involving IRWSTFunctor the change will affect most of the instances so I wanted to double check here before trying it. Do you see it as feasible ? Thanks a lot for the help.

barambani · 2018-03-20T10:13:07Z

I'm not sure what happened with this build, it looks like it was killed. Do I have any way to restart the CI ? Thanks

barambani · 2018-03-27T15:47:44Z

Could the CI be reran ? Thanks

barambani · 2018-04-23T17:16:36Z

This has been quiet for a while. I think I changed any case I could find. It should be ready for a review. Many thanks.

LukaJCB · 2018-04-23T19:18:07Z

Sorry for not reviewing sooner, this looks good to me, apart from the one snippet in WriterTContravariant that you mentioned 👍 Thanks very much! :)

barambani · 2018-04-23T19:50:14Z

No problem at all 😄 , thank you. I will see if I can do something then.

kailuowang · 2018-04-25T01:41:31Z

Thanks so much! sorry about the delay.

barambani · 2018-04-25T09:47:31Z

No worries. Thank you all 👍

Applied suggestion about extending Functor implementation from the

a0dea48

traverse for monad transformer

Addressed binary compatibility

8c37a23

kailuowang previously approved these changes Mar 13, 2018

View reviewed changes

Reused existing instance in other cases following the suggestions from

107635a

the code review

barambani dismissed kailuowang’s stale review via 107635a March 14, 2018 18:39

barambani commented Mar 14, 2018

View reviewed changes

Trying to improve coverage

ba04a97

Extended SemigroupK from Alternative in IndexedReaderWriterStateT

680a842

Reverted contravariant change because of compat.

a178ec5

kailuowang approved these changes Apr 25, 2018

View reviewed changes

LukaJCB approved these changes Apr 25, 2018

View reviewed changes

kailuowang merged commit eba9a59 into typelevel:master May 3, 2018

kailuowang added this to the 1.2 milestone May 30, 2018

kailuowang added the enhancement label May 30, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Extend Functor implementation in the Traverse instance of EitherT, OptionT and Tuple2K #2191

Extend Functor implementation in the Traverse instance of EitherT, OptionT and Tuple2K #2191

barambani commented Mar 12, 2018

kailuowang commented Mar 13, 2018

barambani commented Mar 13, 2018

codecov-io commented Mar 13, 2018 •

edited

Loading

peterneyens commented Mar 13, 2018 •

edited

Loading

barambani commented Mar 13, 2018

barambani Mar 14, 2018 •

edited

Loading

barambani commented Mar 14, 2018

barambani commented Mar 20, 2018

barambani commented Mar 27, 2018 •

edited

Loading

barambani commented Apr 23, 2018

LukaJCB commented Apr 23, 2018

barambani commented Apr 23, 2018

kailuowang commented Apr 25, 2018

barambani commented Apr 25, 2018

Extend Functor implementation in the Traverse instance of EitherT, OptionT and Tuple2K #2191

Extend Functor implementation in the Traverse instance of EitherT, OptionT and Tuple2K #2191

Conversation

barambani commented Mar 12, 2018

kailuowang commented Mar 13, 2018

barambani commented Mar 13, 2018

codecov-io commented Mar 13, 2018 • edited Loading

Codecov Report

peterneyens commented Mar 13, 2018 • edited Loading

barambani commented Mar 13, 2018

barambani Mar 14, 2018 • edited Loading

Choose a reason for hiding this comment

barambani commented Mar 14, 2018

barambani commented Mar 20, 2018

barambani commented Mar 27, 2018 • edited Loading

barambani commented Apr 23, 2018

LukaJCB commented Apr 23, 2018

barambani commented Apr 23, 2018

kailuowang commented Apr 25, 2018

barambani commented Apr 25, 2018

codecov-io commented Mar 13, 2018 •

edited

Loading

peterneyens commented Mar 13, 2018 •

edited

Loading

barambani Mar 14, 2018 •

edited

Loading

barambani commented Mar 27, 2018 •

edited

Loading