Implement a ReaderWriterStateT data type #1598

iravid · 2017-04-08T20:57:29Z

Resolves #1597.

Still missing:

Method parity with ReaderT
MonadWriter, MonadReader instances
tests and laws
scaladoc

I'd love to get some feedback on the structure; this is mostly just a re-implementation of methods from StateT, WriterT and ReaderT, which was great for understanding how everything is wired up and is probably more efficient, but I do wonder about the maintainability of this approach.

codecov-io · 2017-04-08T21:41:07Z

Codecov Report

Merging #1598 into master will increase coverage by 0.05%.
The diff coverage is 94.39%.

@@            Coverage Diff             @@
##           master    #1598      +/-   ##
==========================================
+ Coverage   93.33%   93.38%   +0.05%     
==========================================
  Files         240      241       +1     
  Lines        3946     4053     +107     
  Branches      142      135       -7     
==========================================
+ Hits         3683     3785     +102     
- Misses        263      268       +5

Impacted Files	Coverage Δ
core/src/main/scala/cats/data/package.scala	`85.71% <100%> (+2.38%)`	⬆️
laws/src/main/scala/cats/laws/discipline/Eq.scala	`84.37% <83.33%> (-0.25%)`	⬇️
.../src/main/scala/cats/data/ReaderWriterStateT.scala	`95% <95%> (ø)`
core/src/main/scala/cats/MonadWriter.scala	`33.33% <0%> (+33.33%)`	⬆️

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 e5ff7d1...28acb4e. Read the comment docs.

kailuowang · 2017-04-09T16:43:47Z

related #1210

kailuowang · 2017-04-10T15:45:10Z

@iravid, @edmundnoble is working on a new typeclass encoding for better support MTL, #1210 and some discussion here at gitter provide more context.

iravid · 2017-04-10T15:58:33Z

Thanks. How should this data type be implemented in light of the new design? Does the new design affect only the typeclass instances or the actual data type as well?

…

________________________________ From: Kai(luo) Wang <notifications@github.com> Sent: Monday, April 10, 2017 6:45:12 PM To: typelevel/cats Cc: Itamar Ravid; Mention Subject: Re: [typelevel/cats] Implement a ReaderWriterStateT data type (#1598) @iravid<https://github.com/iravid>, @edmundnoble<https://github.com/edmundnoble> is working on a new typeclass encoding for better support MTL, #1210<#1210> and some discussion here at gitter<https://gitter.im/typelevel/cats?at=58e82851408f90be669c7822> provide more context. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub<#1598 (comment)>, or mute the thread<https://github.com/notifications/unsubscribe-auth/AARSxMyO2ByFm6ikWHP0FTkc3sNv-PH4ks5ruk6IgaJpZM4M30_p>.

edmundnoble · 2017-04-10T20:36:55Z

Just the instances @iravid don't worry, I don't foresee any other changes

iravid · 2017-04-12T08:27:37Z

@kailuowang can we aim for this to go into 1.0 as well?

kailuowang · 2017-04-12T13:29:59Z

@iravid sure. I only included stuff I liked and/or breaking change. We haven't started the process of surveying what people want for 1.0.0-MF yet (we probably should soon, although I kind of want to figure out the biggest uncertainty - MTL refactor first).

iravid · 2017-04-12T14:15:09Z

👍🏻

…

________________________________ From: Kai(luo) Wang <notifications@github.com> Sent: Wednesday, April 12, 2017 4:30:00 PM To: typelevel/cats Cc: Itamar Ravid; Mention Subject: Re: [typelevel/cats] Implement a ReaderWriterStateT data type (#1598) @iravid<https://github.com/iravid> sure. I only included stuffs I liked and/or breaking change. We haven't started the process of surveying what people want for 1.0.0-MF yet (we probably should soon, although I kind of want to figure out the biggest uncertainty - MTL refactor first). — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub<#1598 (comment)>, or mute the thread<https://github.com/notifications/unsubscribe-auth/AARSxNHzokzUtosl8HVqp0VXHIoktJSmks5rvNHYgaJpZM4M30_p>.

wedens · 2017-04-12T15:18:49Z

core/src/main/scala/cats/Unapply.scala

@@ -180,3 +180,20 @@ private[cats] sealed abstract class Unapply3Instances {
    def subst: F[AA, BB, C] => M[C] = identity
  }
 }
+
+private[cats] sealed abstract class Unapply5Instances {


Unapply machinery is about to be removed #1583 /cc @kailuowang

iravid · 2017-04-12T15:48:06Z

Yeah, I'll drop that commit after that is merged. Added it temporarily so I could test `traverse` :-)

________________________________ From: wedens <notifications@github.com> Sent: Wednesday, April 12, 2017 6:18:51 PM To: typelevel/cats Cc: Itamar Ravid; Mention Subject: Re: [typelevel/cats] Implement a ReaderWriterStateT data type (#1598) @wedens commented on this pull request.

________________________________ In core/src/main/scala/cats/Unapply.scala<#1598 (comment)>:

@@ -180,3 +180,20 @@ private[cats] sealed abstract class Unapply3Instances {

def subst: F[AA, BB, C] => M[C] = identity } } + +private[cats] sealed abstract class Unapply5Instances { Unapply machinery is about to be removed #1583<#1583> /cc @kailuowang<https://github.com/kailuowang> — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub<#1598 (review)>, or mute the thread<https://github.com/notifications/unsubscribe-auth/AARSxJn6FUT7HZzN2DRmG0ibA76YHPJiks5rvOtbgaJpZM4M30_p>.

iravid · 2017-04-14T09:34:40Z

This is now ready for review.

iravid · 2017-04-20T16:22:15Z

Any chance this could get reviewed?

kailuowang · 2017-04-20T20:10:24Z

@iravid I haven't looked at this yet, but I am curious if this better resides in the new cats-mtl/transmogrify. see #1616 cc @edmundnoble

iravid · 2017-04-21T05:25:26Z

Happy to move it there, but as this is a concrete data type it seems to me to be better suited to `cats.data`.

wedens · 2017-04-21T05:52:21Z

I agree that cats-mtl/transmogrifier should be for typeclasses, not for specific data structures.

iravid · 2017-04-21T09:35:55Z

Rebased and removed Unapply instance that was added

kailuowang · 2017-04-21T15:16:05Z

@iravid @wedens my secret agenda is more about keeping cats.core lean. This data type is clearly useful, I am curious is it more useful with MTL typeclasses (I never used it for real)? Obviously if it's commonly used without MTL typeclasses it makes sense for it to stay here.

iravid · 2017-04-21T16:16:11Z

I can only speak for myself - I found a need for it in a recent project, in which I would've used it as a data type without MTL :-)

…

________________________________ From: Kai(luo) Wang <notifications@github.com> Sent: Friday, April 21, 2017 6:16:07 PM To: typelevel/cats Cc: Itamar Ravid; Mention Subject: Re: [typelevel/cats] Implement a ReaderWriterStateT data type (#1598) @iravid<https://github.com/iravid> @wedens<https://github.com/wedens> my secret agenda is more about keeping cats.core lean. This data type is clearly useful, I am curious is it more useful with MTL typeclasses (I never used it for real)? Obviously if it's commonly used without MTL typeclasses it makes sense for it to stay here. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub<#1598 (comment)>, or mute the thread<https://github.com/notifications/unsubscribe-auth/AARSxFLxZxQ6oth4B2_886pNioOpsOuPks5ryMg3gaJpZM4M30_p>.

iravid · 2017-05-05T16:03:29Z

Ping :-) Can we push this forwards?

kailuowang · 2017-05-05T16:07:41Z

@iravid, not sure if you noticed, build on scala 2.10.6 is broken
UPDATE: it's due to the fact that the build was taking too long. For now I am going to restart it.

iravid · 2017-05-05T16:28:59Z

Thanks!

iravid · 2017-05-05T20:03:11Z

Looks green, fortunately :-)

kailuowang · 2017-05-05T22:55:33Z

@iravid first of all thanks a lot for being patient with us and this big contribution. Right now we have quite a PR backlog. I am not familiar with the subject enough to review this (although if you want, adding some documentation to the datatypes section on the website might help, e.g. use cases, examples, paper reference etc.). If you'd like to accelerate the review process, mentioning it on gitter might help.

iravid · 2017-05-06T06:25:24Z

Sure, I'll try asking for a review on gitter.

edmundnoble

After a first pass, this looks good. Since it's so large, I'll do another tomorrow.

edmundnoble · 2017-05-06T06:29:57Z

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

+  implicit def L: Monoid[L]
+  type TC[F[_]] = Applicative[F]
+
+  def liftT[F[_], A](fa: F[A])(implicit F: Applicative[F]): RWST[F, E, S, L, A] =


This is just duplicated from RWST.lift.

Right! Will delegate.

edmundnoble

Only possible things I can think of are tests that ask >> ask has the same result as ask, or the reader law, and that local actually modifies the context. Otherwise very well done, and great coverage.

iravid · 2017-05-09T10:37:10Z

Thanks @edmundnoble. I'll add those additional tests and the Reader law and ping you.

edmundnoble

@iravid The tests I mentioned are actually MonadReader laws, which you test already. LGTM.

iravid · 2017-05-10T05:46:54Z

Good point ;) Been a while since I looked at the code. Thanks! @kailuowang I assume we need another 👍 to merge, right?

kailuowang · 2017-05-10T11:19:29Z

@iravid hmm, for a PR as large as this, we could use more pairs of eyeballs.

We are currently following a practice of requiring at least two sign-offs to merge PRs (and for large or contentious issues we may wait for more).

iravid · 2017-05-10T12:01:28Z

Sure - the more, the merrier ;)

…

On 10 May 2017, at 14:19, Kai(luo) Wang ***@***.***> wrote: @iravid <https://github.com/iravid> hmm, for a PR as large as this, we could use more pairs of eyeballs. We are currently following a practice of requiring at least two sign-offs to merge PRs (and for large or contentious issues we may wait for more). — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1598 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AARSxKwtgf1u30sieojC9o8sNALI7UlNks5r4Z1CgaJpZM4M30_p>.

djspiewak · 2017-05-12T05:35:01Z

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

+  * In other words, it is a pre-baked stack of `[[ReaderT]][F, E, A]`, `[[WriterT]][F, L, A]`
+  * and `[[StateT]][F, S, A]`.
+  */
+final class RWST[F[_], E, S, L, A](val runF: F[(E, S) => F[(L, S, A)]]) extends Serializable {


Would it be better to have ReaderWriterStateT be the canonical name, with RWST as an alias? I look at this as being similar to FunctionK and ~>.

djspiewak · 2017-05-12T05:35:58Z

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

+    * Combine this computation with `rwsb` using `fn`. The state will be be threaded
+    * through the computations and the log values will be combined.
+    */
+  def map2[B, Z](rwsb: RWST[F, E, S, L, B])(fn: (A, B) => Z)(implicit F: FlatMap[F], L: Semigroup[L]): RWST[F, E, S, L, Z] =


Is there an advantage to having this as a separate implementation, rather than just straight delegating to flatMap?

No, that's an excellent point.

djspiewak · 2017-05-12T05:36:54Z

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

+import cats.syntax.either._
+
+/**
+  * Represents a stateful computation in a context `F[_]`, over state `S`, with an initial environment `E`, an accumulated log `L` and a result `A`.


Minor thing, but I believe in cats the convention is javadoc-style doc comments, rather than scaladoc-style. Thusly:

/** * Docs here */

Rather than

/** * Boo */

Resolves typelevel#1597.

iravid · 2017-05-12T07:51:03Z

Addressed @djspiewak's comments

ceedubs · 2017-05-13T19:13:29Z

Wow @iravid this is fantastic. Sorry for letting this hang for quite a while.

The only change that I could think of was potentially changing get to getS, but after writing those comments I remembered that ask is used for Reader, so I think that get is probably fine. 👍 from me thanks a lot!

ceedubs · 2017-05-13T19:09:51Z

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

+  /**
+   * Get the input state, without modifying it.
+   */
+  def get(implicit F: Functor[F]): ReaderWriterStateT[F, E, S, L, S] =


The corresponding method in State is named get, but for RWST I wonder if getS (or perhaps getState) might be a better name. In particular, this will help distinguish between getting the input E and getting the input S.

ceedubs · 2017-05-13T19:10:45Z

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

+  /**
+   * Return the input state without modifying it.
+   */
+  def get[F[_], E, S, L](implicit F: Applicative[F], L: Monoid[L]): ReaderWriterStateT[F, E, S, L, S] =


Along the lines of my other comment, I wonder if this should be getS.

ceedubs · 2017-05-13T19:11:26Z

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

+  /**
+   * Return the input state without modifying it.
+   */
+  def get[E, S, L: Monoid]: ReaderWriterState[E, S, L, S] =


Same as others :)

ceedubs · 2017-05-13T19:14:33Z

@djspiewak @kailuowang could you please take another look now that I think all review suggestions have been addressed?

djspiewak · 2017-05-13T22:52:08Z

@ceedubs In my original review, the only thoughts/reservations I had were expressed, and have now been fixed. So I'm 👍 now.

iravid · 2017-05-14T06:57:12Z

One thing that should be changed is switching the TransLift instance for MonadTrans if that PR is merged before this one.

edmundnoble · 2017-05-14T07:06:53Z

@iravid No need to worry about that :)

iravid force-pushed the feature/rwst branch from 0deb39f to 40ae965 Compare April 8, 2017 21:00

iravid force-pushed the feature/rwst branch 2 times, most recently from 4019c35 to a88ef45 Compare April 12, 2017 06:51

kailuowang added this to the 1.0.0-MF milestone Apr 12, 2017

wedens reviewed Apr 12, 2017

View reviewed changes

iravid force-pushed the feature/rwst branch from a88ef45 to 98d6f8b Compare April 14, 2017 09:34

iravid force-pushed the feature/rwst branch 3 times, most recently from c205e5a to 8601f42 Compare April 14, 2017 14:49

iravid force-pushed the feature/rwst branch 2 times, most recently from 035fab0 to 3a27f48 Compare April 21, 2017 09:35

edmundnoble reviewed May 6, 2017

View reviewed changes

iravid force-pushed the feature/rwst branch from 3a27f48 to 9c7fd7f Compare May 6, 2017 07:13

edmundnoble suggested changes May 9, 2017

View reviewed changes

edmundnoble approved these changes May 9, 2017

View reviewed changes

djspiewak reviewed May 12, 2017

View reviewed changes

Implement a ReaderWriterStateT data type

efda442

Resolves typelevel#1597.

iravid force-pushed the feature/rwst branch from 9c7fd7f to efda442 Compare May 12, 2017 05:49

Itamar Ravid added 3 commits May 12, 2017 09:18

Re-align the stars

3d2e1df

Rename to ReaderWriterStateT and add aliases

118fe21

Replace map2 implementation with flatMap delegation, remove map2Eval

28acb4e

ceedubs approved these changes May 13, 2017

View reviewed changes

edmundnoble merged commit 586ea08 into typelevel:master May 14, 2017

peterneyens mentioned this pull request May 17, 2017

Replace TransLift with MonadTrans #1621

Merged

anler mentioned this pull request May 26, 2017

MonadState instance for ReaderT/Kleisli #1704

Closed

Implement a ReaderWriterStateT data type #1598

Implement a ReaderWriterStateT data type #1598

Conversation

iravid commented Apr 8, 2017 • edited Loading

codecov-io commented Apr 8, 2017 • edited Loading

Codecov Report

kailuowang commented Apr 9, 2017

kailuowang commented Apr 10, 2017

iravid commented Apr 10, 2017 via email

edmundnoble commented Apr 10, 2017

iravid commented Apr 12, 2017

kailuowang commented Apr 12, 2017 • edited Loading

iravid commented Apr 12, 2017 via email

Choose a reason for hiding this comment

iravid commented Apr 12, 2017 via email

iravid commented Apr 14, 2017

iravid commented Apr 20, 2017

kailuowang commented Apr 20, 2017 • edited Loading

iravid commented Apr 21, 2017 via email • edited Loading

wedens commented Apr 21, 2017

iravid commented Apr 21, 2017

kailuowang commented Apr 21, 2017

iravid commented Apr 21, 2017 via email

iravid commented May 5, 2017

kailuowang commented May 5, 2017 • edited Loading

iravid commented May 5, 2017

iravid commented May 5, 2017

kailuowang commented May 5, 2017

iravid commented May 6, 2017

edmundnoble left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

edmundnoble left a comment

Choose a reason for hiding this comment

iravid commented May 9, 2017

edmundnoble left a comment

Choose a reason for hiding this comment

iravid commented May 10, 2017

kailuowang commented May 10, 2017

iravid commented May 10, 2017 via email

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

iravid commented May 12, 2017

ceedubs commented May 13, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ceedubs commented May 13, 2017

djspiewak commented May 13, 2017

iravid commented May 14, 2017

edmundnoble commented May 14, 2017

iravid commented Apr 8, 2017 •

edited

Loading

codecov-io commented Apr 8, 2017 •

edited

Loading

kailuowang commented Apr 12, 2017 •

edited

Loading

kailuowang commented Apr 20, 2017 •

edited

Loading

iravid commented Apr 21, 2017 via email •

edited

Loading

kailuowang commented May 5, 2017 •

edited

Loading