Skip to content

Add FunctorFilter and TraverseFilter - #1225

Merged
ceedubs merged 8 commits into
typelevel:masterfrom
ceedubs:traverse-filter
Jul 28, 2016
Merged

Add FunctorFilter and TraverseFilter#1225
ceedubs merged 8 commits into
typelevel:masterfrom
ceedubs:traverse-filter

Conversation

@ceedubs

@ceedubs ceedubs commented Jul 23, 2016

Copy link
Copy Markdown
Contributor

This is a replacement for #1148.

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 filterM broken? #1054.
  • They provide collect which is a handy method that is often used on
    standard library collections but had been absent in Cats.

`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.
@codecov-io

codecov-io commented Jul 23, 2016

Copy link
Copy Markdown

Current coverage is 90.13% (diff: 96.11%)

Merging #1225 into master will increase coverage by 0.13%

@@             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          

Sunburst

Powered by Codecov. Last update 106e114...5a67c7f

@ceedubs

ceedubs commented Jul 23, 2016

Copy link
Copy Markdown
Contributor Author

Oops I have a couple things to add. Closing for the moment.

@ceedubs ceedubs closed this Jul 23, 2016
@ceedubs ceedubs reopened this Jul 23, 2016
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, ?]] {

Copy link
Copy Markdown
Contributor

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?

Copy link
Copy Markdown
Contributor Author

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.

@kailuowang

Copy link
Copy Markdown
Contributor

Again, this looks awesome. Thank you @ceedubs 👍

@typeclass trait MonadFilter[F[_]] extends Monad[F] {
@typeclass trait MonadFilter[F[_]] extends Monad[F] with FunctorFilter[F] {

def empty[A]: F[A]

Copy link
Copy Markdown
Contributor

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.

Copy link
Copy Markdown
Contributor Author

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.

@ceedubs

ceedubs commented Jul 24, 2016

Copy link
Copy Markdown
Contributor Author

I've updated with a couple typo fixes based on feedback. I've also added a TraverseFilter instance for Map.

@adelbertc

Copy link
Copy Markdown
Contributor

Wow, that's quite a lot of work. LGTM 👍

@kailuowang

Copy link
Copy Markdown
Contributor

👍

@johnynek

Copy link
Copy Markdown
Contributor

👍 Thanks for the hard work here indeed.

@ceedubs

ceedubs commented Jul 28, 2016

Copy link
Copy Markdown
Contributor Author

Thanks for the great review comments, @johnynek, @kailuowang, and @mikejcurry!

@ceedubs
ceedubs merged commit 5b35e84 into typelevel:master Jul 28, 2016
@ceedubs
ceedubs deleted the traverse-filter branch July 28, 2016 01:51
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.

filterM broken?

7 participants