Skip to content
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

Add Random materializers for monad transformers #1850

Merged
merged 2 commits into from
Apr 20, 2021

Conversation

alexandrustana
Copy link
Contributor

This closes #1834

Copy link
Member

@djspiewak djspiewak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is great! Just a couple super-minor changes. I'm thinking that the mima compatibility failures are spurious? Need to look at them a little closer and could use a second set of eyes.

Comment on lines 233 to 234
implicit def catsStateTRandom[F[_]: Random: Applicative, S]: Random[StateT[F, S, *]] =
Random[F].mapK(StateT.liftK)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should probably be IndexedStateT just to be slightly more general

* [[Random]] instance built for `cats.data.ReaderWriterStateT` values
* initialized with any `F` data type that also implements `Random`.
*/
implicit def catsReaderWriterStateTRandom[
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also Indexed, ideally

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wasn't aware of the Indexed version, should be ok now 🙂

@wemrysi
Copy link
Contributor

wemrysi commented Apr 10, 2021

@djspiewak I agree the mima issues are spurious, looks like the PR branch is based on a series/3.x pre #1854.

@alexandrustana
Copy link
Contributor Author

@wemrysi you are right it's before #1854. I'll get the fix in when applying the requested changes

@djspiewak djspiewak merged commit 76729eb into typelevel:series/3.x Apr 20, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add inductive materializations for Random
3 participants