Add `Foldable`, `Traverse` and `Comonad` instances to `WriterT` #2182

barambani · 2018-03-04T21:39:24Z

This adds Foldable, Traverse and Comonad instances for WriterT. The relevant issue is #620

codecov-io · 2018-03-04T22:19:17Z

Codecov Report

Merging #2182 into master will decrease coverage by 0.02%.
The diff coverage is 85.71%.

@@            Coverage Diff             @@
##           master    #2182      +/-   ##
==========================================
- Coverage   94.75%   94.73%   -0.03%     
==========================================
  Files         330      330              
  Lines        5568     5582      +14     
  Branches      201      204       +3     
==========================================
+ Hits         5276     5288      +12     
- Misses        292      294       +2

Impacted Files	Coverage Δ
core/src/main/scala/cats/data/WriterT.scala	`91.37% <85.71%> (-1.76%)`	⬇️
...rc/main/scala/cats/laws/discipline/Arbitrary.scala	`100% <0%> (+1.28%)`	⬆️

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 9135035...152ee7a. Read the comment docs.

johnynek · 2018-03-04T23:33:44Z

I’m surprised this passes binary compatibility, but assuming that’s correct I’m +1

johnynek · 2018-03-04T23:35:41Z

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

@@ -63,6 +63,17 @@ final case class WriterT[F[_], L, V](run: F[(L, V)]) {
    mapWritten(_ => monoidL.empty)

  def show(implicit F: Show[F[(L, V)]]): String = F.show(run)
+
+  def foldLeft[C](c: C)(f: (C, V) => C)(implicit F: Foldable[F]): C =
+    F.foldLeft(run, c)((z, v) => f(z, v._2))


Can we call v instead lv here to remind us it has the written value?

Sure thing, thanks

johnynek · 2018-03-04T23:36:22Z

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

+    F.foldLeft(run, c)((z, v) => f(z, v._2))
+
+  def foldRight[C](lc: Eval[C])(f: (V, Eval[C]) => Eval[C])(implicit F: Foldable[F]): Eval[C] =
+    F.foldRight(run, lc)((v, z) => f(v._2, z))


Can we call v as lv here too?

barambani · 2018-03-05T00:38:51Z

@johnynek not sure why it passes, I'm refreshing right now the rules of binary compatibility. What's the change here that you are not 100% sure about ? Because I might work it around even if it passes. Thanks for the help.

kailuowang · 2018-03-05T15:26:29Z

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

@@ -95,9 +106,21 @@ private[data] sealed abstract class WriterTInstances extends WriterTInstances0 {
      implicit val F0: Monad[F] = F
      implicit val L0: Monoid[L] = L
    }
+
+  implicit def catsDataFoldableForWriterT[F[_], L, V](implicit F: Foldable[F]): Foldable[WriterT[F, L, ?]] =


The Traverse instance is more specific and thus should have a higher priority than the Foldable instance.
In case you are curious, here is the guideline in this regard.
https://typelevel.org/cats/guidelines.html#a-idimplicit-priority-hrefimplicit-prioritya-implicit-instance-priority
I guess you can leave the instance of Traverse where you have now and move the Foldable instance down to WriterTInstance1

thanks, makes perfectly sense. I wasn't sure about the precedence there, I should have read the guidelines myself. I'm changing it.

kailuowang · 2018-03-05T15:28:18Z

I believe the renaming of package private classes is binary compatible. If that's what @johnynek was referring to.

kailuowang

thanks very much!

barambani · 2018-03-05T17:02:07Z

Thank you lads for helping through the process. 👍

barambani · 2018-03-07T12:12:31Z

Considering that this is still open I would add the Comonad instance to this PR if that's OK

LukaJCB · 2018-03-07T12:39:06Z

Sounds good :)

kailuowang · 2018-03-07T18:59:40Z

@barambani , I took the liberty to update the PR title, if you don't mind.

barambani · 2018-03-07T19:05:44Z

Thanks for doing it. I will also make the description more accurate

ceedubs · 2018-03-09T04:04:44Z

Can you add resolution tests for Writer (that is, when F is Id)? I suspect that it currently fails to resolve.

ceedubs · 2018-03-09T04:05:36Z

And thanks a lot by the way. This is great!

barambani · 2018-03-09T14:00:33Z

You're right, that's not resolved. I'm adding the tests and the fix. Thanks a lot

parameters.

barambani · 2018-03-09T16:25:05Z

Just for my understanding, could you point me to some details about why scalac fails to summon F[_] instances for Id ? I know that the de-aliasing is pretty aggressive, so that might be a reason, but I would like to know more. Thanks a lot.

kailuowang · 2018-03-09T17:27:27Z

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

@@ -95,9 +106,23 @@ private[data] sealed abstract class WriterTInstances extends WriterTInstances0 {
      implicit val F0: Monad[F] = F
      implicit val L0: Monoid[L] = L
    }
+
+  implicit def catsDataTraverseForWriterTId[L](implicit F: Traverse[Id]): Traverse[WriterT[Id, L, ?]] =


@barambani you can't have any instance that potentially conflicts in the same trait level.
I think there is Functor instance conflict here for [WriterT[Id, L, ?]. I think you might need to create quite a few more priority layers to separate the instances.

to be clear I didn't see the actually error message on travis. But I think separating them out might solve the problem.

Sorry for the confusion, my bad for the lack of clarity. This prioritisation should work (let's see when the CI completes). My question was more in general about the reason why we needed to create instances for Id. I would have expected that something like Comonad[WriterT[Id, ListWrapper[Int], ?]] could summon the instance for F[_] and I was wondering if you had resources about why that's not the case.
This doesn't mean that I'm not happy to add more priority levels if you think that's better. Thanks for the great help here.

@barambani I don't know the underlying reason that Id needs to be special-cased, but it's a weakness in scalac's type inference. This is a related Stack Overflow post that I've seen before. Something like Id might be a tricky one because A =:= Id[A] =:= Id[Id[A]], so there may be some logic to short-circuit inference when applying a type results in the same type so you don't end up in an infinite loop or something. This is pure speculation :)

@ceedubs thanks a lot, I didn't want for you to waste time on this. I was just taking advantage of this discussion in case you knew it out of your head 😃 👍

ceedubs

👍 LGTM thanks a lot, @barambani!

peterneyens

Thanks @barambani, I left two small comments.

peterneyens · 2018-03-12T08:35:16Z

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

+  def foldRight[A, B](fa: WriterT[F, L, A], lb: Eval[B])(f: (A, Eval[B]) => Eval[B]): Eval[B] = fa.foldRight(lb)(f)
+}
+
+private[data] sealed trait WriterTTraverse[F[_], L] extends Traverse[WriterT[F, L, ?]] with WriterTFoldable[F, L] {


If you extends WriterTFunctor here as well, you get its map implementation instead of the default traverse[Id] from Traverse.

I will change it thanks 👍

peterneyens · 2018-03-12T08:43:19Z

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

+
+  def traverse[G[_], V1](f: V => G[V1])(implicit F: Traverse[F], G: Applicative[G]): G[WriterT[F, L, V1]] =
+    G.map(
+      F.traverse(run)(lv => G.product(G.pure(lv._1), f(lv._2)))


Can this be G.tupleLeft(f(lv._2), lv._1) instead of G.product(G.pure(lv._1), f(lv._2))?

sure changing it 👍

peterneyens · 2018-03-12T13:10:18Z

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


  override implicit def F0: Traverse[F]

+  override def map[A, B](fa: WriterT[F, L, A])(f: A => B): WriterT[F, L, B] = super[WriterTFunctor].map(fa)(f)


I think we can omit this line if we defined map in WriterTFunctor as override def map.

See for example EitherTFunctor and EitherTMonad (although EitherTTraverse doesn't extends EitherTFunctor at the moment and neither does OptionTTraverse OptionTFunctor).

Very true, apologies. I'm fixing it. I rushed it in. I have to stop reading those intellij errors. Do you think I should change also EitherTTraverse and OptionTTraverse accordingly ?

I think there are probably more cases than just EitherTTraverse and OptionTTraverse which can be improved, I would leave those for another PR.

LukaJCB

LGTM :)

peterneyens

Thanks again @barambani (and for the quick follow up on my nitpicking).

barambani added 5 commits March 3, 2018 20:29

Added foldLeft and foldRight to WriterT and Foldable instance

0f3b16c

Added tests for Foldable instance

4d1d7f7

Added traverse. Fixed priority of the implicit search for isntances

241f213

Added traverse implementation to WriterT

e68849d

Added test for WriterT Traverse

6729608

johnynek reviewed Mar 4, 2018

View reviewed changes

Refactored names

2a7f5f2

kailuowang suggested changes Mar 5, 2018

View reviewed changes

Changed priority of Traverse instance

454cc8f

kailuowang previously approved these changes Mar 5, 2018

View reviewed changes

kailuowang added this to the 1.1 milestone Mar 5, 2018

barambani added 2 commits March 7, 2018 09:40

Added comonad instance for WrtiterT and Const. Added tests

4cf8304

Completed Comonad tests using tuple instead of Const

0c2c002

barambani dismissed kailuowang’s stale review via 0c2c002 March 7, 2018 14:20

Refactored extract derivation to avoid implicit search

e63a7e3

kailuowang previously approved these changes Mar 7, 2018

View reviewed changes

kailuowang changed the title ~~Issue 620~~ Add Foldable, Traverse and Comonad instances to WriterT Mar 7, 2018

Added instances and reolution tests for Id. Removed unneeded type

fba0f7a

parameters.

barambani dismissed kailuowang’s stale review via fba0f7a March 9, 2018 16:21

Minor refactoring

2e4b2b5

kailuowang reviewed Mar 9, 2018

View reviewed changes

barambani added 2 commits March 9, 2018 21:28

Attempt to increase coverage

fd077c3

Removed unneeded serialization test

a40e9f7

ceedubs previously approved these changes Mar 10, 2018

View reviewed changes

peterneyens reviewed Mar 12, 2018

View reviewed changes

Refactored traverse derivation

b3ab664

barambani dismissed ceedubs’s stale review via b3ab664 March 12, 2018 11:33

peterneyens reviewed Mar 12, 2018

View reviewed changes

Fixed improper inheritance of WriterTFunctor

152ee7a

LukaJCB previously approved these changes Mar 12, 2018

View reviewed changes

barambani dismissed LukaJCB’s stale review via 152ee7a March 12, 2018 15:21

peterneyens approved these changes Mar 12, 2018

View reviewed changes

kailuowang merged commit 23e0a72 into typelevel:master Mar 12, 2018

ghost mentioned this pull request Mar 12, 2018

split build jvm,add workers to kernel #2189

Merged

barambani deleted the issue-620-foldable branch March 12, 2018 22:01

barambani mentioned this pull request Mar 12, 2018

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

Merged

kailuowang added the enhancement label Mar 16, 2018


		override implicit def F0: Traverse[F]

		override def map[A, B](fa: WriterT[F, L, A])(f: A => B): WriterT[F, L, B] = super[WriterTFunctor].map(fa)(f)

Add Foldable, Traverse and Comonad instances to WriterT #2182

Add Foldable, Traverse and Comonad instances to WriterT #2182

Conversation

barambani commented Mar 4, 2018 • edited Loading

codecov-io commented Mar 4, 2018 • edited Loading

Codecov Report

johnynek commented Mar 4, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

barambani commented Mar 5, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kailuowang commented Mar 5, 2018

kailuowang left a comment

Choose a reason for hiding this comment

barambani commented Mar 5, 2018

barambani commented Mar 7, 2018

LukaJCB commented Mar 7, 2018

kailuowang commented Mar 7, 2018

barambani commented Mar 7, 2018

ceedubs commented Mar 9, 2018

ceedubs commented Mar 9, 2018

barambani commented Mar 9, 2018

barambani commented Mar 9, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

barambani Mar 9, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ceedubs left a comment

Choose a reason for hiding this comment

peterneyens left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LukaJCB left a comment

Choose a reason for hiding this comment

peterneyens left a comment

Choose a reason for hiding this comment

Add `Foldable`, `Traverse` and `Comonad` instances to `WriterT` #2182

Add `Foldable`, `Traverse` and `Comonad` instances to `WriterT` #2182

barambani commented Mar 4, 2018 •

edited

Loading

codecov-io commented Mar 4, 2018 •

edited

Loading

barambani Mar 9, 2018 •

edited

Loading