-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Clean up ReaderWriterStateT #1706
Conversation
- Check if type class instances are serializable - Move Arbitrary instance to cats-laws - Move unamibiguous type class instances
rwsb.map { b => | ||
fn(a, b) | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can get the same from Apply#map2
The version from Apply#map2
would look like :
map(flatMap(fa)(a => map(fb)(b => (a, b)))) { case (a, b) => f(a, b) }
If we want the simpler implementation used here in RWST
, we could override map2
in FlatMap
.
|
||
// check serializable using Option | ||
checkAll("MonadReader[ReaderWriterStateT[Option, String, Int, String, ?], String]", | ||
SerializableTests.serializable(MonadReader[ReaderWriterStateT[Option, String, Int, String, ?], String])) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I used ListWrapper
here I got
Message: cannot assign instance of scala.collection.immutable.List$SerializationProxy to field cats.tests.ListWrapper.list of type scala.collection.immutable.List in instance of cats.tests.ListWrapper
So I used Option
like the analogous test for Kleisli
.
Codecov Report
@@ Coverage Diff @@
## master #1706 +/- ##
==========================================
- Coverage 93.96% 93.95% -0.01%
==========================================
Files 241 241
Lines 4091 4089 -2
Branches 160 156 -4
==========================================
- Hits 3844 3842 -2
Misses 247 247
Continue to review full report at Codecov.
|
Thanks for doing this @peterneyens! I agree we should address these before 1.0.0-MF |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍
I also find the ordering of the type parameters a bit confusing. Now it is
E
(Reader
) ->S
(State
) ->L
(Writer
), while the nameReaderWriterStateT
suggests thatWriter
comes beforeState
. The reason is probably theBifunctor
instance for theWriter
part.I would drop the
Bifunctor
instance and switch the order ofS
andL
so it aligns with the name. We could also leave as it is or change toReaderStateWriterT
😃. If we want to change something, it should probably be done before 1.0.0-MF. Opinions?