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

Get FunctorFilter on par with the other MTL typeclasses #2594

Merged
merged 3 commits into from
Nov 19, 2018
Merged

Get FunctorFilter on par with the other MTL typeclasses #2594

merged 3 commits into from
Nov 19, 2018

Conversation

barambani
Copy link
Contributor

@barambani barambani commented Nov 4, 2018

This adds FunctorFilter to Kleisli and IndexedStateT as from the issue #2556 . As a side note I tried to reduce the requirements for the IndexedStateT instance but I couldn't come up with anything that need less than Applicative and FlatMap, so I kept the implementation in the issue that requires Monad (see below as an example)

def mapFilter[A, B](fa: IndexedStateT[F, SA, SB, A])(f: A => Option[B]): IndexedStateT[F, SA, SB, B] =
  IndexedStateT[F, SA, SB, B] { sa =>
    FF.mapFilter(fa.run(sa)){ case (sb, a) => f(a) map (sb -> _) }
  }

I'm happy to change it though if there's something less restrictive.
There is still some work to do about the comments at the bottom of the ticket that I'm still investigating. I need more details there.

Also I'm planning to open some follow up PRs to add instances of TraverseFilter as well to Kleisli and IndexedStateT, and to add both to other transformers where they are still missing. Is it something that's needed ? Happy to stop if they are not a priority.

@codecov-io
Copy link

codecov-io commented Nov 4, 2018

Codecov Report

Merging #2594 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2594      +/-   ##
==========================================
+ Coverage   95.16%   95.16%   +<.01%     
==========================================
  Files         361      361              
  Lines        6634     6641       +7     
  Branches      294      301       +7     
==========================================
+ Hits         6313     6320       +7     
  Misses        321      321
Impacted Files Coverage Δ
core/src/main/scala/cats/data/Kleisli.scala 97.24% <100%> (+0.1%) ⬆️
core/src/main/scala/cats/data/IndexedStateT.scala 89.89% <100%> (+0.31%) ⬆️

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 1aac375...d9bcf99. Read the comment docs.

@barambani
Copy link
Contributor Author

As from #2556 (comment) the PR is ready for review.

@Baccata
Copy link
Contributor

Baccata commented Nov 5, 2018

Sorry for the misleading comment, it looks like EitherT needs one too 😅

@barambani
Copy link
Contributor Author

Thanks @Baccata , very true. There are some other that are still missing it and there might be also some that need a TraverseFilter instance. At the moment it is only provided for Option, OptionT and few other. As I commented above I would address them in a separate PR, but I want to make sure first that there is intention to have them.

@barambani
Copy link
Contributor Author

@kailuowang , @LukaJCB do you have any bandwidth to have a look at this PR ? In case let me know if you'd like for me to add anything here. Thanks a lot.

@kailuowang
Copy link
Contributor

sorry for the late response. I will take a look Monday

@barambani
Copy link
Contributor Author

no worries. Many thanks

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.

Thanks so much!

@barambani
Copy link
Contributor Author

👍

@kailuowang kailuowang added this to the 1.5.0-RC1 milestone Nov 19, 2018
@kailuowang kailuowang merged commit d7e9e99 into typelevel:master Nov 19, 2018
@barambani barambani deleted the issue-2556 branch November 19, 2018 16:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants