-
-
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
Add FunctorFilter and TraverseFilter #1225
Conversation
`TraverseFilter` is a port of Haskell's Data.Witherable, and represents structures that support doing a combined `traverse` and `filter` as a single operation. `FunctorFilter` is similar but is limited to a combined `map` and `filter` as a single operation. The main reason that I've added `FunctorFilter` is to extract the commonality between `TraverseFilter` and `MonadFilter` so that they don't have collisions in methods such as `filter`. Some benefits of these type classes: - They provide `filterM` (actually a more general version of it - `filterA`), which resolves typelevel#1054. - They provide `collect` which is a handy method that is often used on standard library collections but had been absent in Cats.
Current coverage is 90.13% (diff: 96.11%)@@ master #1225 diff @@
==========================================
Files 235 243 +8
Lines 3201 3284 +83
Methods 3146 3228 +82
Messages 0 0
Branches 52 53 +1
==========================================
+ Hits 2881 2960 +79
- Misses 320 324 +4
Partials 0 0
|
Oops I have a couple things to add. Closing for the moment. |
implicit def catsDataTraverseForConst[C]: Traverse[Const[C, ?]] = new Traverse[Const[C, ?]] { | ||
def traverse[G[_]: Applicative, A, B](fa: Const[C, A])(f: A => G[B]): G[Const[C, B]] = | ||
fa.traverse(f) | ||
implicit def catsDataTraverseForConst[C]: TraverseFilter[Const[C, ?]] = new TraverseFilter[Const[C, ?]] { |
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.
nit: does the name need to be consistent with the pattern, i.e. catsDataTraverseFilterForConst
?
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.
Good catch. I think I might have this in a couple places in this PR. These changes area already not binary compatible, so there's no reason not to change this.
Again, this looks awesome. Thank you @ceedubs 👍 |
* sense for that particular monad). This is of particular interest to | ||
* us since it allows us to add a `filter` method to a Monad, which is | ||
* used when pattern matching or using guards in for comprehensions. | ||
*/ | ||
@typeclass trait MonadFilter[F[_]] extends Monad[F] { | ||
@typeclass trait MonadFilter[F[_]] extends Monad[F] with FunctorFilter[F] { | ||
|
||
def empty[A]: F[A] |
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.
It seems like a FunctorFilter had to have an empty value too. We could just filter everything from any value and get there, no?
Seems like empty might almost be there.
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.
We could just filter everything from any value and get there, no?
That's true, but you would have to have a value to start with. With Applicative
you have pure
for that, but with Functor
this isn't available.
I was thinking about whether the empty
from MonadFilter
should be moved to FunctorFilter
along with the filter
method at first, but I think this constrains FunctorFilter
more than we might want. For example, that would mean that the FunctorFilter
instance for OptionT[F, A]
would then require an Applicative[F]
as opposed to the Functor[F]
that it currently requires. And more generally, I think that what I have in this PR as a composeFilter
method on Functor
would need to instead be moved to Applicative
.
I think that if we were to add an ApplicativeFilter
, that would be the place to add the def empty[A]: F[A]
. With ApplicativeFilter
, you could even implement empty
and pure
in terms of each other (with help from map
).
At this point I haven't added ApplicativeFilter
because I didn't have a particularly compelling use-case for it and there's already a lot going on in this type class hierarchy. However, I should be able to add it if people have good use-cases for it.
I've updated with a couple typo fixes based on feedback. I've also added a |
Wow, that's quite a lot of work. LGTM 👍 |
👍 |
👍 Thanks for the hard work here indeed. |
Thanks for the great review comments, @johnynek, @kailuowang, and @mikejcurry! |
This is something that I missed on typelevel#1225.
This is a replacement for #1148.
TraverseFilter
is a port of Haskell's Data.Witherable, and representsstructures that support doing a combined
traverse
andfilter
as asingle operation.
FunctorFilter
is similar but is limited to a combinedmap
andfilter
as a single operation. The main reason that I've addedFunctorFilter
is to extract the commonality betweenTraverseFilter
and
MonadFilter
so that they don't have collisions in methods such asfilter
.Some benefits of these type classes:
filterM
(actually a more general version of it -filterA
), which resolves filterM broken? #1054.collect
which is a handy method that is often used onstandard library collections but had been absent in Cats.