Add `traverseCollect` to `TraverseFilter` typeclass #4277

emilhotkowski · 2022-07-30T08:23:45Z

Thank you for contributing to Cats!

This is a kind reminder to run sbt +prePR and commit the changed files, if any, before submitting.

armanbilge

Thanks for the PR, looking good! As next steps, do you think we can add a law, syntax (and test for the syntax)?

core/src/main/scala/cats/TraverseFilter.scala

emilhotkowski · 2022-07-31T07:01:42Z

I added syntax and test for it, but I am not sure if we need laws. Looking at TraverseFilterLaws, we don't have laws for other functions like sequenceFilter. Should I add some. for traverseCollect anyway?

armanbilge · 2022-07-31T07:42:23Z

Looking at TraverseFilterLaws, we don't have laws for other functions like sequenceFilter. Should I add some. for traverseCollect anyway?

Oh that's interesting. I think laws are good to have, in case an implementation overrides this method, and at the very least to exercise this code. It's also often good to have some tests with a concrete implementation testing it works as expected. Here's a recent PR that is a great example:

Added updated to Traverse #4248

emilhotkowski · 2022-07-31T09:58:49Z

By laws you mean we want to have a test proving that traverseCollect works the same as traverse and collect in two operations?

armanbilge · 2022-07-31T22:37:35Z

The law should verify that the actual implementation of the method matches the default implementation—you can reuse the same code. You might find the commentary in #4248 (comment) helpful :) let me know if you have questions!

emilhotkowski · 2022-08-02T16:47:07Z

I added all necessary stuff (I believe so) :)

laws/src/main/scala/cats/laws/TraverseFilterLaws.scala

laws/src/main/scala/cats/laws/discipline/TraverseFilterTests.scala

johnynek · 2022-08-02T23:49:53Z

thanks for your PR!

armanbilge · 2022-08-02T23:58:55Z

core/src/main/scala/cats/TraverseFilter.scala

@@ -22,6 +22,7 @@
 package cats

 import cats.data.State
+import cats.implicits.toTraverseOps


Can we write this without the ops import? Current practice is try to avoid circular dependencies between syntax and typeclasses.

I tried to do manual sequence for this operation. WDYT?

armanbilge · 2022-08-03T00:18:54Z

core/src/main/scala/cats/TraverseFilter.scala

@@ -69,7 +68,7 @@ trait TraverseFilter[F[_]] extends FunctorFilter[F] {
   * res0: List[Option[String]] = List(Some(two), None)
   */
  def traverseCollect[G[_], A, B](fa: F[A])(f: PartialFunction[A, G[B]])(implicit G: Applicative[G]): G[F[B]] =
-    traverseFilter(fa)(a => f.lift(a).sequence)
+    traverseFilter(fa)(a => f.lift(a).map(G.map(_)(Option(_))).getOrElse(G.pure(None)))


I think sequence was good, we just can't use the syntax :)

Suggested change

traverseFilter(fa)(a => f.lift(a).map(G.map(_)(Option(_))).getOrElse(G.pure(None)))

traverseFilter(fa)(a => Traverse[Option].sequence(f.lift(a)))

actually, while we are at it:

val optF = f.lift traverseFilter(fa)(a => Traverse[Option].sequence(optF(a)))

so we don't reallocate f.lift on every item a.

@emilhotkowski I think Oscar's suggestion still applies :) it's a good optimization.

Co-authored-by: Arman Bilge <armanbilge@gmail.com>

armanbilge · 2022-08-03T11:51:17Z

core/src/main/scala/cats/syntax/traverseFilter.scala

+  implicit def toTraverseCollectOps[F[_], G[_], A](fa: F[A]): TraverseCollectOps[F, G, A] =
+    new TraverseCollectOps(fa)


Should this just be TraverseFilterOps? We can add more ops in the future; it's not specific to traverseCollect I think.

I followed current implementations like new SequenceFilterOps(fgoa) I don't know what would be the other solution tbh

Ah the situation for SequenceFilter is slighly different, since it only works for F[G[Option[A]]], not F[A]

cats/core/src/main/scala/cats/syntax/traverseFilter.scala

Line 28 in 0cd6d46

implicit def toSequenceFilterOps[F[_], G[_], A](fgoa: F[G[Option[A]]]): SequenceFilterOps[F, G, A] =

Do you know where I could find examples to follow? I am not sure what I need to change here.

Sure, check out any of the syntax implementations :)
https://github.com/typelevel/cats/tree/main/core/src/main/scala/cats/syntax

I'm just proposing that we rename TraverseCollectOps to TraverseFilterOps so that it matches the name of the typeclass and not the operation.

I understand your point now. I thought it was a remark about implementation details. Thanks for your work on this PR !

Ah, sorry I wasn't so clear on that. Great, thank you too!

laws/src/main/scala/cats/laws/discipline/TraverseFilterTests.scala

…cala Co-authored-by: Arman Bilge <armanbilge@gmail.com>

armanbilge

LGTM, thanks for this!

Emil Hotkowski added 2 commits July 30, 2022 09:34

#4276 - Added traverseCollect to TraverseFilter typeclass

4f8b7e4

Formatting

4590a07

armanbilge reviewed Jul 30, 2022

View reviewed changes

core/src/main/scala/cats/TraverseFilter.scala Outdated Show resolved Hide resolved

armanbilge reviewed Jul 30, 2022

View reviewed changes

core/src/main/scala/cats/TraverseFilter.scala Show resolved Hide resolved

Added syntax for traverseCollect

c626183

armanbilge changed the title ~~#4276 - Added traverseCollect to TraverseFilter typeclass~~ Add traverseCollect to TraverseFilter typeclass Jul 31, 2022

armanbilge added the enhancement label Jul 31, 2022

armanbilge linked an issue Jul 31, 2022 that may be closed by this pull request

Add traverseCollect to TraverseFilter typeclass #4276

Closed

armanbilge added this to the 2.9.0 milestone Jul 31, 2022

Emil Hotkowski added 2 commits August 1, 2022 23:47

Added proper example

f73a186

Added reference law

a226d01

johnynek requested changes Aug 2, 2022

View reviewed changes

laws/src/main/scala/cats/laws/TraverseFilterLaws.scala Outdated Show resolved Hide resolved

Emil Hotkowski added 2 commits August 2, 2022 23:39

Added test for a law

25e8bbe

Formatting

9455986

emilhotkowski requested review from armanbilge and johnynek August 2, 2022 22:11

Fixed unidoc example

2e250c5

johnynek reviewed Aug 2, 2022

View reviewed changes

laws/src/main/scala/cats/laws/discipline/TraverseFilterTests.scala Outdated Show resolved Hide resolved

laws/src/main/scala/cats/laws/discipline/TraverseFilterTests.scala Outdated Show resolved Hide resolved

Emil Hotkowski added 2 commits August 3, 2022 00:59

Removed unnused props

6afcfaf

Added implicit arbitrary

b2d2d67

emilhotkowski requested a review from johnynek August 2, 2022 23:18

formatting

d1fa2f4

johnynek previously approved these changes Aug 2, 2022

View reviewed changes

armanbilge reviewed Aug 2, 2022

View reviewed changes

Remove dependency on ops

cdc49db

emilhotkowski dismissed johnynek’s stale review via cdc49db August 3, 2022 00:15

armanbilge reviewed Aug 3, 2022

View reviewed changes

Update core/src/main/scala/cats/TraverseFilter.scala

fd87685

Co-authored-by: Arman Bilge <armanbilge@gmail.com>

emilhotkowski requested a review from armanbilge August 3, 2022 05:39

small optimisation for optF

0cd6d46

armanbilge reviewed Aug 3, 2022

View reviewed changes

Change to more general name

cf96c2b

emilhotkowski requested review from armanbilge and johnynek August 16, 2022 20:16

armanbilge reviewed Aug 17, 2022

View reviewed changes

laws/src/main/scala/cats/laws/discipline/TraverseFilterTests.scala Outdated Show resolved Hide resolved

Update laws/src/main/scala/cats/laws/discipline/TraverseFilterTests.s…

8b7dcb6

…cala Co-authored-by: Arman Bilge <armanbilge@gmail.com>

emilhotkowski dismissed a stale review via 8b7dcb6 August 17, 2022 08:49

armanbilge approved these changes Aug 17, 2022

View reviewed changes

johnynek approved these changes Aug 17, 2022

View reviewed changes

armanbilge merged commit 791c65d into typelevel:main Aug 17, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `traverseCollect` to `TraverseFilter` typeclass #4277

Add `traverseCollect` to `TraverseFilter` typeclass #4277

emilhotkowski commented Jul 30, 2022

armanbilge left a comment

emilhotkowski commented Jul 31, 2022

armanbilge commented Jul 31, 2022

emilhotkowski commented Jul 31, 2022

armanbilge commented Jul 31, 2022

emilhotkowski commented Aug 2, 2022

johnynek commented Aug 2, 2022

armanbilge Aug 2, 2022

emilhotkowski Aug 3, 2022

armanbilge Aug 3, 2022

johnynek Aug 3, 2022

armanbilge Aug 3, 2022

emilhotkowski Aug 3, 2022

armanbilge Aug 3, 2022

emilhotkowski Aug 3, 2022

armanbilge Aug 3, 2022

emilhotkowski Aug 15, 2022

armanbilge Aug 16, 2022 •

edited

Loading

emilhotkowski Aug 16, 2022

armanbilge Aug 17, 2022

armanbilge left a comment

	traverseFilter(fa)(a => f.lift(a).map(G.map(_)(Option(_))).getOrElse(G.pure(None)))
	traverseFilter(fa)(a => Traverse[Option].sequence(f.lift(a)))

		implicit def toTraverseCollectOps[F[_], G[_], A](fa: F[A]): TraverseCollectOps[F, G, A] =
		new TraverseCollectOps(fa)

Add traverseCollect to TraverseFilter typeclass #4277

Add traverseCollect to TraverseFilter typeclass #4277

Conversation

emilhotkowski commented Jul 30, 2022

armanbilge left a comment

Choose a reason for hiding this comment

emilhotkowski commented Jul 31, 2022

armanbilge commented Jul 31, 2022

emilhotkowski commented Jul 31, 2022

armanbilge commented Jul 31, 2022

emilhotkowski commented Aug 2, 2022

johnynek commented Aug 2, 2022

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

armanbilge Aug 16, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

armanbilge left a comment

Choose a reason for hiding this comment

Add `traverseCollect` to `TraverseFilter` typeclass #4277

Add `traverseCollect` to `TraverseFilter` typeclass #4277

armanbilge Aug 16, 2022 •

edited

Loading