Convert ReaderWriterStateT to IndexedReaderWriterStateT - #1803
Conversation
5fd991e to
c90849c
Compare
Codecov Report
@@ Coverage Diff @@
## master #1803 +/- ##
==========================================
+ Coverage 95.35% 95.49% +0.13%
==========================================
Files 248 248
Lines 4396 4418 +22
Branches 108 119 +11
==========================================
+ Hits 4192 4219 +27
+ Misses 204 199 -5
Continue to review full report at Codecov.
|
bfbffdd to
2a1689e
Compare
|
All feedback appreciated, folks |
|
One small thing: I think we should rename the file this is in to match its name (e.g. rename |
|
Thanks for the comment - I’ll do that once review finishes, as it makes the diffs much harder to read :-) |
| private[data] sealed trait RWSTInstances extends RWSTInstances1 { | ||
| implicit def catsDataProfunctorForRWST[F[_], L, S](implicit F0: Functor[F]): Profunctor[ReaderWriterStateT[F, ?, L, S, ?]] = | ||
| new RWSTProfunctor[F, L, S] { | ||
| private[data] sealed trait IRWSTInstances extends IRWSTInstances1 { |
There was a problem hiding this comment.
per #1879 do you mind change all the instance traits here into abstract class? I am creating a PR to change other classes, but don't want to touch this one to avoid conflict.
|
Ping. This has been sitting here for a while. Any chance we could move this forward? |
johnynek
left a comment
johnynek
left a comment
There was a problem hiding this comment.
looks good. Seems like a mechanical change. The shuffling of the methods in the functions is a bit of a pain to review, but I guess it can't be helped.
Made one comment asking if this can be an Arrow now.
| * and `IndexedStateT[F, SA, SB, A]`. | ||
| */ | ||
| final class ReaderWriterStateT[F[_], E, L, S, A](val runF: F[(E, S) => F[(L, S, A)]]) extends Serializable { | ||
| final class IndexedReaderWriterStateT[F[_], E, L, SA, SB, A](val runF: F[(E, SA) => F[(L, SB, A)]]) extends Serializable { |
There was a problem hiding this comment.
it feels like this could have an Arrow[SA, SB] instance now. Am I wrong?
I don't know if it is worth it, but it if it can have an instance, it would be nice to add (same may be true for IndexedStateT).
There was a problem hiding this comment.
Hmm, we can definitely do Compose and Strong.
Category (id) and Arrow (lift) are a bit more problematic. We need to conjure up the value somehow. That could work for V: Monoid.
I'll add Compose+Strong and Arrow as separate traits. Thanks for the suggestion
There was a problem hiding this comment.
@johnynek Added the instances both here and in the IndexedStateT PR.
ec240cf to
0870bdd
Compare
|
👍 |
|
LGTM, I guess it's time to change the filenames. |
0870bdd to
893c4b6
Compare
|
Hmm, I see that coverage has decreased somehow. I'll fix that. |
|
Ok, 100% diff coverage, we should be good to go :-) |
|
Thanks so much @iravid. Sorry it dragged so long. |
|
Thanks @kailuowang, looking forward to using this when 1.0.0-RC1 is out :-) |
5 participants
Resolves #1774, #1786.
In favour of consistency with #1775, the following has changed:
transformShas been added with a similar signatureIRWST.contramapnow transforms the input state and so does theContravariantinstance, accordinglyIRWST.dimapnow transforms the input and output states, and so does theProfunctorinstanceIRWST.bimapand aBifunctorinstance has been added to transform the resulting output state and output value