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

Clean up ReaderWriterStateT #1706

Merged
merged 1 commit into from
Jun 1, 2017
Merged

Conversation

peterneyens
Copy link
Collaborator

  • Check if type class instances are serializable
  • Move Arbitrary instance to cats-laws
  • Move unambiguous type class instances

I also find the ordering of the type parameters a bit confusing. Now it is E (Reader) -> S (State) -> L (Writer), while the name ReaderWriterStateT suggests that Writer comes before State. The reason is probably the Bifunctor instance for the Writer part.

I would drop the Bifunctor instance and switch the order of S and L so it aligns with the name. We could also leave as it is or change to ReaderStateWriterT 😃. If we want to change something, it should probably be done before 1.0.0-MF. Opinions?

- Check if type class instances are serializable
- Move Arbitrary instance to cats-laws
- Move unamibiguous type class instances
rwsb.map { b =>
fn(a, b)
}
}
Copy link
Collaborator Author

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]))
Copy link
Collaborator Author

@peterneyens peterneyens May 27, 2017

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-io
Copy link

codecov-io commented May 27, 2017

Codecov Report

Merging #1706 into master will decrease coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            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
Impacted Files Coverage Δ
...rc/main/scala/cats/laws/discipline/Arbitrary.scala 90.47% <100%> (+0.15%) ⬆️
.../src/main/scala/cats/data/ReaderWriterStateT.scala 95.87% <100%> (-0.13%) ⬇️

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 80d56e6...d3edd4f. Read the comment docs.

@kailuowang kailuowang added this to the 1.0.0-MF milestone May 28, 2017
@kailuowang
Copy link
Contributor

Thanks for doing this @peterneyens! I agree we should address these before 1.0.0-MF

@kailuowang kailuowang mentioned this pull request May 28, 2017
26 tasks
Copy link
Contributor

@kailuowang kailuowang left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@kailuowang kailuowang merged commit 4845e68 into typelevel:master Jun 1, 2017
@peterneyens peterneyens deleted the RWST-cleanup branch June 1, 2017 14:46
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.

5 participants