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

Override some methods in Kleisli instances #1618

Merged
merged 2 commits into from
Apr 21, 2017

Conversation

peterneyens
Copy link
Collaborator

  • Switch the order of the traits so that some methods are overridden by the implementation in other type class instances in its hierarchy

    For example:

    • extends Monad[Kleisli[F, A, ?]] with KleisliApplicative[F, A]
    • instead of extends KleisliApplicative[F, A] with Monad[Kleisli[F, A, ?]])

    We were already overriding ap in Monad, this also overrides product (which was the initial reason for this PR).
    For the rest we can just loose a couple of explicit overrides (eg super[KleisliSplit].split(f, g))

  • Add a MonadReader test for Reader, which improves some code coverage.

- Switch the order of the traits so that some methods are overridden by
  the implementation in other type class instances in its hierarchy.
- Add a `MonadReader` test for `Reader`
@codecov-io
Copy link

codecov-io commented Apr 19, 2017

Codecov Report

Merging #1618 into master will decrease coverage by 0.02%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1618      +/-   ##
==========================================
- Coverage   93.11%   93.08%   -0.03%     
==========================================
  Files         250      250              
  Lines        3992     3979      -13     
  Branches      138      141       +3     
==========================================
- Hits         3717     3704      -13     
  Misses        275      275
Impacted Files Coverage Δ
core/src/main/scala/cats/data/Kleisli.scala 92.64% <100%> (-1.19%) ⬇️

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 4e381d2...91cf873. Read the comment docs.

@ceedubs
Copy link
Contributor

ceedubs commented Apr 20, 2017

Thanks @peterneyens!

This generally looks really good to me. One minor question: was there any particular motivation for changing a bunch of implicit parameter names to something like ev? I was thinking that in most other places in cats we were using implicit parameter names like F, M, AE etc, but admittedly I haven't looked around much to see if that's true.

@peterneyens
Copy link
Collaborator Author

Most of the parameters here in Kleisli were called ev, but it is probably best to be consistent in general, so I switched them around.

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 added this to the 1.0.0-MF milestone Apr 20, 2017
@kailuowang kailuowang merged commit c1dcd40 into typelevel:master Apr 21, 2017
@peterneyens peterneyens deleted the peter-kleisli-override branch April 21, 2017 07:17
@kailuowang kailuowang modified the milestone: 1.0.0-MF Apr 26, 2017
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