Add FunctorFilter and TraverseFilter #2405

LukaJCB · 2018-08-15T14:10:23Z

Should fix #2348. Needs to be coordinated with their removal from cats-mtl :)

codecov-io · 2018-08-15T16:38:13Z

Codecov Report

Merging #2405 into master will increase coverage by 0.1%.
The diff coverage is 100%.

@@            Coverage Diff            @@
##           master    #2405     +/-   ##
=========================================
+ Coverage   95.29%   95.39%   +0.1%     
=========================================
  Files         351      357      +6     
  Lines        6371     6516    +145     
  Branches      286      288      +2     
=========================================
+ Hits         6071     6216    +145     
  Misses        300      300

Impacted Files	Coverage Δ
core/src/main/scala/cats/data/Const.scala	`100% <100%> (ø)`	⬆️
core/src/main/scala/cats/instances/list.scala	`100% <100%> (ø)`	⬆️
core/src/main/scala/cats/instances/option.scala	`100% <100%> (ø)`	⬆️
...ala/cats/laws/discipline/TraverseFilterTests.scala	`100% <100%> (ø)`
core/src/main/scala/cats/FunctorFilter.scala	`100% <100%> (ø)`
core/src/main/scala/cats/instances/map.scala	`94.59% <100%> (+1.04%)`	⬆️
core/src/main/scala/cats/data/OptionT.scala	`96.9% <100%> (+0.39%)`	⬆️
core/src/main/scala/cats/data/Nested.scala	`95.12% <100%> (+0.67%)`	⬆️
.../src/main/scala/cats/laws/TraverseFilterLaws.scala	`100% <100%> (ø)`
...s/src/main/scala/cats/laws/FunctorFilterLaws.scala	`100% <100%> (ø)`
... and 12 more

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 77c9319...b4c5080. Read the comment docs.

LukaJCB · 2018-08-16T07:16:44Z

core/src/main/scala/cats/FunctorEmpty.scala

+
+object FunctorEmpty {
+  implicit def catsFunctorForFunctorEmpty[F[_]](fe: FunctorEmpty[F]): Functor[F] =
+    fe.functor


This function here allows us to call things like FunctorEmpty[F].map, do you think this is worthwhile? I feel like this is an easy change that doesn't cause any ambiguities and let's us atleast have some of the niceties that we lose when doing composition instead of inheritance :)

I don’t think I like it it’s an unusual use and only saving one .functor in there.

Alternatively could we makes functor provide this as a lower priority?

I am not sure if it's needed either.
What would be the use case of it?
If user import FunctorEmpty._ and all of a sudden whenever there is a FunctorEmpty in scope there is also a Functor in scope, then will there be ambiguities with other possible Functor instances?
Should user be just writing the following instead?
def foo[F[_]: FunctorEmpty : Functor]: ....

There won't be ambiguities, because only an explicit instance will be converted, notice it is (fe: FunctorEmpty[F]): Functor[F] and not (implicit fe: FunctorEmpty[F]): Functor[F].

The only real benefit is that when you have a method like this:

def foo[F[_]](fa: F[Int])(implicit F: FunctorEmpty[F]): F[String] = F.map(fa)(_.toString)

You'll be able to call map et al. directly on it instead of doing F.functor.map. So the benefit is rather small, but at the same time it doesn't cause ambiguities. Also note that import FunctorEmpty._ won't actually do anything as this instance will always be in scope whenever FunctorEmpty is in scope :)

ah, sorry I missed that fe isn't implicit. As you said, the benefit is a bit small, especially since you usually don't need to write implicit F: FunctorEmpty[F] at the first place, you could just use context bound, so not much typing to be saved.
I'd prefer encouraging users to the more conventional type class syntax enabled style.

def foo[F[_]: FunctorEmpty : Functor](fa: F[Int]): F[String] = { fa.map(_.toString).filter(_.length > 2) }

Sounds good to me :)

… filter-classes

LukaJCB · 2018-08-16T08:21:43Z

tests/src/test/scala/cats/tests/ChainSuite.scala

@@ -25,6 +25,9 @@ class ChainSuite extends CatsSuite {
  checkAll("Chain[Int]", OrderTests[Chain[Int]].order)
  checkAll("Order[Chain]", SerializableTests.serializable(Order[Chain[Int]]))

+  checkAll("Chain[Int]", TraverseEmptyTests[Chain].traverseEmpty[Int, Int, Int])
+  checkAll("TraverseEmpty[Chain]", SerializableTests.serializable(TraverseEmpty[Chain]))


For some reason TraverseEmpty[Chain] seems not to be able to be serialized, though I really don't understand why. Any ideas?

I think that I got it -- LukaJCB#4

I think that the issue here is that it was referencing an instance-level `catsDataInstancesForChain` from an abstract class. By changing it to reference `Chain.catsDataInstancesForChain`, it is a reference to a static member (and therefore doesn't actually need to be serialized). Take my explanation with a grain of salt -- like everyone else on the planet, I don't actually understand Java serialization. But at the end of the day it works :)

Make TraverseEmpty[Chain] serializable

johnynek · 2018-08-16T17:15:04Z

core/src/main/scala/cats/FunctorEmpty.scala

+
+object FunctorEmpty {
+  implicit def catsFunctorForFunctorEmpty[F[_]](fe: FunctorEmpty[F]): Functor[F] =
+    fe.functor


I don’t think I like it it’s an unusual use and only saving one .functor in there.

Alternatively could we makes functor provide this as a lower priority?

johnynek · 2018-08-16T17:20:23Z

core/src/main/scala/cats/FunctorEmpty.scala

+@typeclass
+trait FunctorEmpty[F[_]] extends Serializable {
+  def functor: Functor[F]
+


Why doesn’t this have an empty method with the law that anything filtered out becomes empty?

Also, if you have a FunctorEmpty and MonoidK empty should be consistent.

Can we define any extra methods if we require implementors to implement an empty method? Or just for the laws? The Haskell precedent doesn't seem to define a method like this and I'm not sure if we should either, need to think about what the actual benefits are 🤔

well, I guess if you have an empty, since you can't combine it, you are always stuck with an empty. But still, that's interesting for laws empty.map(fn) == empty etc...

I hate to have a hacky way to make an empty, but not provide it (namely give me any instance, and I will filter it).

Here's some discussion on it #1365 and here's the haskell counter part http://hackage.haskell.org/package/witherable-0.2/docs/Data-Witherable.html

The original PR also has some discussion on why it wasn't included in the first place https://github.com/typelevel/cats/pull/1225/files#discussion_r71987653

More specifically, this comment: #1225 (comment)

Yeah after thinking about it more a bit, I think I'd vote to keep it as is without the empty method.

rossabaker · 2018-08-28T15:22:06Z

core/src/main/scala/cats/TraverseEmpty.scala

+trait TraverseEmpty[F[_]] extends FunctorEmpty[F] {
+  def traverse: Traverse[F]
+
+  override def functor: Functor[F] = traverse


I haven't spent a lot of time on the this type of encoding. Is there a reason that functor should ever not be the traverse? In other words, do we lose anything by making this final?

Nope, I agree fully, I'll change it :)

non · 2018-08-28T15:32:20Z

I understand the arguments against providing empty -- but given that what we have is a functor that can be filtered, I'd prefer a name like FunctorFilter or FilterableFunctor or something similar. I think mentioning empty in the name is not a good idea if we don't want to require that method.

LukaJCB · 2018-08-28T15:42:11Z

@non I'm fine with renaming them back to TraverseFilter and FunctorFilter as they were originally called. :)

ceedubs · 2018-08-31T17:58:26Z

When I originally added these in #1225, I had some doctest examples. It would be nice to see those come back. Also is any of the compose work in that PR still relevant?

LukaJCB · 2018-09-02T08:06:09Z

Hi Cody, I'll add those doctests back :)
W.r.t to compose, I just added instances for Nested, not sure if we also need the explicit compose 🤔

ceedubs · 2018-09-02T14:32:07Z

core/src/main/scala/cats/data/EitherT.scala

+      (implicit G: Applicative[G]): G[EitherT[F, L, B]] =
+        G.map(
+          F0.traverseFilter[G, Either[L, A], Either[L, B]](fa.value) {
+            case l@Left(_) => G.pure(Option(l.rightCast[B]))


The Option.apply call here is either doing an unnecessary null-check here or is masking a null. I think that we should just use the Some constructor instead (and I believe that we have followed this convention elsewhere in Cats).

ceedubs · 2018-09-02T14:50:52Z

core/src/main/scala/cats/data/EitherT.scala

+      (fa: EitherT[F, L, A])
+      (f: A => G[Boolean])
+      (implicit G: Applicative[G]): G[EitherT[F, L, A]] =
+        G.map(F0.filterA(fa.value)(_.fold(_ => G.pure(true), f)))(EitherT[F, L, A])


Here and in the filter method, it isn't obvious to me that a Left should result in true as opposed to false. Is there a law that guides us in this direction? If not, was there a particular reason for this choice? I'm not saying that I think that it should be the other way around; just that I wouldn't have known which one to pick. And for the Nested instances, we use a Traverse for the outer type and a TraverseFilter for the inner type, so it seems a little weird to me that you couldn't get a TraverseFilter instance for an F[Either[L, ?]] but if you lift it into an EitherT you can get one. This makes me question the validity/consistency/intuitiveness of this instance.

This is just the implementation we had in cats-mtl, so I'm not sure what the motivation was. I agree that it seems weird though, maybe we should delete it. Did you provide one in your initial PR?

No, I didn't. I'd be inclined to leave it out until someone makes a good argument for it.

Yeah agree, can always add it back later 👍

ceedubs · 2018-09-02T14:57:44Z

core/src/main/scala/cats/instances/option.scala

+
+    def traverseFilter[G[_], A, B](fa: Option[A])(f: (A) => G[Option[B]])(implicit G: Applicative[G]): G[Option[B]] =
+      fa match {
+        case _: None.type => G.pure(Option.empty[B])


Minor: I think that this could just be case None

ceedubs · 2018-09-02T14:57:57Z

core/src/main/scala/cats/instances/option.scala

+
+    override def filterA[G[_], A](fa: Option[A])(f: (A) => G[Boolean])(implicit G: Applicative[G]): G[Option[A]] =
+      fa match {
+        case _: None.type => G.pure(Option.empty[A])


Same comment as above

ceedubs · 2018-09-02T15:05:02Z

core/src/main/scala/cats/data/OptionT.scala

@@ -234,6 +234,20 @@ private[data] sealed abstract class OptionTInstances extends OptionTInstances0 {
      def defer[A](fa: => OptionT[F, A]): OptionT[F, A] =
        OptionT(F.defer(fa.value))
    }
+
+  implicit def optionTFunctorFilter[F[_]: Functor]: FunctorFilter[OptionT[F, ?]] = {


There was a TraverseFilter instance for OptionT in #1225. Is there an issue with it?

No idea, just took what was there from cats-mtl

I'm having trouble tracking down the history in cats-mtl, but I think that we should have the OptionT instance. It should be have identically to the corresponding Nested instance.

LukaJCB · 2018-09-02T18:30:12Z

@ceedubs I pushed a new commit, addressing your points :)

ceedubs

Glad to see these returning. Thanks @LukaJCB!

ceedubs · 2018-09-02T19:11:32Z

core/src/main/scala/cats/data/EitherT.scala

@@ -514,6 +514,8 @@ private[data] abstract class EitherTInstances extends EitherTInstances1 {
      def defer[A](fa: => EitherT[F, L, A]): EitherT[F, L, A] =
        EitherT(F.defer(fa.value))
    }
+
+


Minor: extra newline here

ceedubs · 2018-09-02T19:11:43Z

core/src/main/scala/cats/data/Nested.scala

+      implicit val G: TraverseFilter[G] = G0
+    }
+
+


Minor: extra newline here.

LukaJCB force-pushed the filter-classes branch from 022ea7c to 23688dc Compare August 15, 2018 15:54

kailuowang added this to the 1.3 milestone Aug 15, 2018

LukaJCB changed the title ~~Add FunctorEmpty and TraverseEmpty~~ WIP: Add FunctorEmpty and TraverseEmpty Aug 15, 2018

LukaJCB force-pushed the filter-classes branch 2 times, most recently from b0cd563 to 9c9d9de Compare August 15, 2018 16:44

Add FunctorEmpty and TraverseEmpty

9c9d9de

LukaJCB changed the title ~~WIP: Add FunctorEmpty and TraverseEmpty~~ Add FunctorEmpty and TraverseEmpty Aug 15, 2018

kailuowang mentioned this pull request Aug 15, 2018

Add Align typeclass (#1263) #1755

Closed

LukaJCB force-pushed the filter-classes branch from 895fcd8 to f97ba06 Compare August 16, 2018 06:39

Luka Jacobowitz added 2 commits August 16, 2018 08:40

Merge branch 'master' into filter-classes

f97ba06

Merge branch 'master' into filter-classes

4810a9f

LukaJCB commented Aug 16, 2018

View reviewed changes

Luka Jacobowitz added 2 commits August 16, 2018 09:28

Add TraverseEmpty instance for Chain

d77ab2a

Merge branch 'filter-classes' of https://github.com/LukaJCB/cats into…

54903f6

… filter-classes

LukaJCB commented Aug 16, 2018

View reviewed changes

LukaJCB requested review from ceedubs and johnynek August 16, 2018 13:59

ceedubs and others added 2 commits August 16, 2018 07:39

Merge pull request #4 from ceedubs/fix-chain-traverse-empty-ctcization

344810e

Make TraverseEmpty[Chain] serializable

LukaJCB mentioned this pull request Aug 16, 2018

Remove FunctorEmpty and TraverseEmpty in conjunction with cats adding them typelevel/cats-mtl#47

Closed

johnynek reviewed Aug 16, 2018

View reviewed changes

Luka Jacobowitz and others added 2 commits August 16, 2018 19:36

Merge branch 'master' into filter-classes

b9e337a

Remove conversion to mainline classes

a3f0d26

stephen-lazaro mentioned this pull request Aug 21, 2018

Remove FunctorEmpty and TraverseEmpty typelevel/cats-mtl#51

Merged

Add traverseFilter <-> traverse consistency law

d323da2

rossabaker reviewed Aug 28, 2018

View reviewed changes

Make Functor override final

8255d59

LukaJCB changed the title ~~Add FunctorEmpty and TraverseEmpty~~ Add FunctorFilter and TraverseFilter Aug 28, 2018

Rename Functor and Traverse Empty to Filter

bf07dd3

LukaJCB force-pushed the filter-classes branch from 4b65c9a to bf07dd3 Compare August 28, 2018 17:07

LukaJCB force-pushed the filter-classes branch from ea4753d to 96e3cc8 Compare September 2, 2018 08:12

Merge branch 'master' into filter-classes

716d701

LukaJCB force-pushed the filter-classes branch from 96e3cc8 to 716d701 Compare September 2, 2018 09:18

Add Nested FunctorFilter instance

46f2307

ceedubs suggested changes Sep 2, 2018

View reviewed changes

ceedubs previously approved these changes Sep 2, 2018

View reviewed changes

Address feedback

b4c5080

LukaJCB dismissed ceedubs’s stale review via b4c5080 September 3, 2018 07:01

LukaJCB force-pushed the filter-classes branch from 568daf4 to b4c5080 Compare September 3, 2018 07:01

ceedubs approved these changes Sep 3, 2018

View reviewed changes

LukaJCB merged commit 2f129c6 into typelevel:master Sep 3, 2018

Add FunctorFilter and TraverseFilter #2405

Add FunctorFilter and TraverseFilter #2405

Conversation

LukaJCB commented Aug 15, 2018

codecov-io commented Aug 15, 2018 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

non commented Aug 28, 2018

LukaJCB commented Aug 28, 2018

ceedubs commented Aug 31, 2018

LukaJCB commented Sep 2, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LukaJCB commented Sep 2, 2018

ceedubs left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented Aug 15, 2018 •

edited

Loading