added `collectFirst` and `collectFirstSome` to `Foldable` #2030

kailuowang · 2017-11-16T16:49:58Z

No description provided.

LukaJCB · 2017-11-16T18:52:47Z

core/src/main/scala/cats/Foldable.scala

+   * {{{
+   * scala> import cats.implicits._
+   * scala> val numbers = List(2,4,5,6,8,10)
+   * scala> numbers.collectFst(i => if(i % 2 == 1) Some(i.toDouble / 2d) else None)


scalastyle wants a space between if and the parantheses.

LukaJCB · 2017-11-16T18:52:55Z

core/src/main/scala/cats/Foldable.scala

+   * scala> val numbers = List(2,4,5,6,8,10)
+   * scala> numbers.collectFst(i => if(i % 2 == 1) Some(i.toDouble / 2d) else None)
+   * res0: Option[Double] = Some(2.5)
+   * scala> List(2, 4, 6).collectFst(i => if(i % 2 == 1) Some(i.toDouble / 2d) else None)


Same as above :)

rossabaker · 2017-11-16T18:59:17Z

I find it confusing to introduce a collectFirst that is different from the standard collectFirst that will shadow it on so many Foldables. I imagine only wanting to call this via the alias.

kailuowang · 2017-11-16T19:18:37Z

@rossabaker that's a valid point. We had the same issue with foldRight as well, in which case we still keep the foldRight name and provide a foldR alias. I don't feel strongly about it, just try to be consistent. If you still prefer I can remove the collectFirst and just keep collectFst

codecov-io · 2017-11-16T19:35:58Z

Codecov Report

Merging #2030 into master will decrease coverage by 0.04%.
The diff coverage is 88.57%.

@@            Coverage Diff             @@
##           master    #2030      +/-   ##
==========================================
- Coverage   95.16%   95.11%   -0.05%     
==========================================
  Files         304      304              
  Lines        5027     5061      +34     
  Branches      121      129       +8     
==========================================
+ Hits         4784     4814      +30     
- Misses        243      247       +4

Impacted Files	Coverage Δ
...eycats-core/src/main/scala/alleycats/std/set.scala	`100% <ø> (ø)`	⬆️
...eycats-core/src/main/scala/alleycats/std/map.scala	`4.76% <0%> (-1.13%)`	⬇️
core/src/main/scala/cats/instances/sortedMap.scala	`95.23% <100%> (+0.32%)`	⬆️
core/src/main/scala/cats/Foldable.scala	`100% <100%> (ø)`	⬆️
core/src/main/scala/cats/instances/queue.scala	`100% <100%> (ø)`	⬆️
core/src/main/scala/cats/instances/stream.scala	`96.61% <100%> (+0.11%)`	⬆️
laws/src/main/scala/cats/laws/FoldableLaws.scala	`100% <100%> (ø)`	⬆️
...ain/scala/cats/laws/discipline/FoldableTests.scala	`100% <100%> (ø)`	⬆️
core/src/main/scala/cats/instances/vector.scala	`72.72% <100%> (+1.29%)`	⬆️
core/src/main/scala/cats/instances/option.scala	`100% <100%> (ø)`	⬆️
... and 2 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 ef64ff8...3d9c390. Read the comment docs.

rossabaker · 2017-11-16T19:39:43Z

My argument is diminished by the fact I've never noticed the foldRight clash. I still think it's confusing, but I feel less strongly than I did two comments ago in light of that.

johnynek · 2017-11-16T20:16:03Z

core/src/main/scala/cats/Foldable.scala

+    }.value
+
+  /**
+   * alias for collectFirst


can we explain why this is here also (not just in the above comment).

johnynek · 2017-11-16T20:16:56Z

core/src/main/scala/cats/Foldable.scala

@@ -215,6 +215,33 @@ import simulacrum.typeclass
      }

  /**
+   * Like `collectFirst` from `scala.collection.Traversable` but takes `A => Option[B]`
+   * instead of `PartialFunction`s.


I think partial functions are very nice for this case. why not use them?

For a PartialFunction, when isDefinedAt and apply relies on a shared computation, that computation will have to be repeated for the hit case? For example, giving a list of keys, you want to find the first key (value) with which you can find a record in a Map. A PartialFunction would course one extra Map query. Also Map.get is just very nature, making it a PartialFunction would be awkward right?

👍 Reviewing some old code, I've implemented it this way on a couple pre-cats projects. I'd rather this suggestion than dropping it altogether.

TraversableOnce does an unspeakable thing to avoid the double evaluation.

@rossabaker wow, that is unspeakable. That certainly removed one of the incentives of adding this method. Just need a opposite version of PartialFunction#lift, maybe in mouse.

Function.unlift?

done! closing this one.

Oh, I thought you were talking about removing the incentive for collectFst. What about Foldable without collectFirst? I've needed it on NonEmptyList before.

My mistake, my only incentive is for a A=>Option[B] collectFirst. I am reopening it. I wonder if we should add collectFirst: PartialFunction[A, B] and collectFirstO: A => Option[B]
For the default collectFirst implementation (which is going to be overriden for actual TraversableOnce data types) shall we do the same trick as TraversableOnce?

johnynek · 2017-11-17T16:14:16Z

I’m +1 on two methods. I’d be more verbose. collectFirstSome or something. Which is what we are doing in the Option returning case.

I’m also +1 on ugly hacks in libraries that speed things up for many users if they are safe. I haven’t studied this particular hack in detail however.

kailuowang · 2017-11-17T17:27:43Z

Just made the change, I can't use the hacks because I couldn't get applyOrElse API to work with foldRight, however I did override it in our instances.

rossabaker · 2017-11-19T21:59:32Z

core/src/main/scala/cats/Foldable.scala

@@ -214,6 +214,31 @@ import simulacrum.typeclass
        case Right(_) => None
      }

+  def collectFirst[A, B](fa: F[A])(pf: PartialFunction[A, B]): Option[B] =


Is it worth mentioning that a more efficient implementation is often possible? We do this on size and foldM, but I suppose it would be easy to get carried away with this. I don't feel strongly.

My battery is going to die before I can run the tests, but I think this might be right:

foldRight(fa, Eval.now(Option.empty[B])) { (a, lb) => var result: Eval[Option[B]] = lb pf.runWith(b => result = Eval.now(Some(b)))(a) result }.value

This is the TraversableOnce trick in foldRight form. TraversableOnce claims it's faster than runWith, but I have not benchmarked it in this context. Tests pass even after I remove the overrides. sentinel could be moved to a private member of the companion.

val sentinel: Function1[A, Any] = new scala.runtime.AbstractFunction1[A, Any]{ def apply(a: A) = this } foldRight(fa, Eval.now(Option.empty[B])) { (a, lb) => val x = pf.applyOrElse(a, sentinel) if (x.asInstanceOf[AnyRef] ne sentinel) Eval.now(Some(x.asInstanceOf[B])) else lb }.value

thanks a lot @rossabaker , I made the change.

johnynek

This looks great to me! Thanks.

johnynek · 2017-11-20T16:08:49Z

core/src/main/scala/cats/Foldable.scala

+   */
+  def collectFirstSome[A, B](fa: F[A])(f: A => Option[B]): Option[B] =
+    foldRight(fa, Eval.now(Option.empty[B])) { (a, lb) =>
+      val fa = f(a)


nit: can we not shadow fa with f(a) which is actually Option[B] and not a F[A]?

oops. fixed.

rossabaker

Moving the sentinel to the companion would save an allocation per invocation of collectFirst, but also makes the hack a little less localized. I'm happy either way.

kailuowang · 2017-11-20T21:44:12Z

@rossabaker I didnt move it because it was parameterized to A, I just realized that it doesn't have to. So I moved it.

rossabaker

I like the solution, and that the git blame of AbstractFunction1[Any, Any] won't be me!

kailuowang added in progress Binary Breaking labels Nov 16, 2017

kailuowang added this to the 1.0.0 milestone Nov 16, 2017

kailuowang requested review from tpolecat and LukaJCB November 16, 2017 16:50

LukaJCB previously approved these changes Nov 16, 2017

View reviewed changes

LukaJCB reviewed Nov 16, 2017

View reviewed changes

kailuowang dismissed LukaJCB’s stale review via b609796 November 16, 2017 18:58

johnynek reviewed Nov 16, 2017

View reviewed changes

kailuowang closed this Nov 16, 2017

kailuowang removed the in progress label Nov 16, 2017

kailuowang reopened this Nov 17, 2017

kailuowang added the in progress label Nov 17, 2017

kailuowang changed the title ~~added a collectFirst that takes A => Option[B] instead of PartialFunction~~ added collectFirst and collectFirstSome to Foldable Nov 18, 2017

rossabaker reviewed Nov 19, 2017

View reviewed changes

added a collectFirst that takes A => Option[B]

492532f

kailuowang force-pushed the collectFirst branch from daa1a0d to 0ee9906 Compare November 20, 2017 15:37

kailuowang added 6 commits November 20, 2017 10:37

added mima exception

ea71fe5

fix scalastyle

639ca2b

update docs

a3257d0

improve doc example

13b032b

address feedback

d4465c6

fix mima

f3805b5

use the performance trick from traverseOnce

0ee9906

LukaJCB previously approved these changes Nov 20, 2017

View reviewed changes

johnynek reviewed Nov 20, 2017

View reviewed changes

address feedback

69f3f85

kailuowang dismissed LukaJCB’s stale review via 69f3f85 November 20, 2017 16:13

fix build

fdc585d

johnynek previously approved these changes Nov 20, 2017

View reviewed changes

minor style change

6e42ea2

kailuowang dismissed johnynek’s stale review via 6e42ea2 November 20, 2017 19:33

rossabaker previously approved these changes Nov 20, 2017

View reviewed changes

move sentinel to companion

3d9c390

kailuowang dismissed rossabaker’s stale review via 3d9c390 November 20, 2017 21:41

rossabaker approved these changes Nov 20, 2017

View reviewed changes

kailuowang merged commit 9325184 into typelevel:master Nov 21, 2017

kailuowang added enhancement and removed in progress labels Nov 21, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

added `collectFirst` and `collectFirstSome` to `Foldable` #2030

added `collectFirst` and `collectFirstSome` to `Foldable` #2030

kailuowang commented Nov 16, 2017

LukaJCB Nov 16, 2017

LukaJCB Nov 16, 2017

rossabaker commented Nov 16, 2017

kailuowang commented Nov 16, 2017 •

edited

Loading

codecov-io commented Nov 16, 2017 •

edited

Loading

rossabaker commented Nov 16, 2017

johnynek Nov 16, 2017

johnynek Nov 16, 2017

kailuowang Nov 16, 2017 •

edited

Loading

rossabaker Nov 16, 2017

rossabaker Nov 16, 2017

kailuowang Nov 16, 2017

rossabaker Nov 16, 2017

kailuowang Nov 16, 2017

rossabaker Nov 16, 2017

kailuowang Nov 17, 2017

johnynek commented Nov 17, 2017

kailuowang commented Nov 17, 2017

rossabaker Nov 19, 2017

rossabaker Nov 19, 2017

rossabaker Nov 20, 2017

kailuowang Nov 20, 2017

johnynek left a comment

johnynek Nov 20, 2017

kailuowang Nov 20, 2017

rossabaker left a comment

kailuowang commented Nov 20, 2017

rossabaker left a comment

added collectFirst and collectFirstSome to Foldable #2030

added collectFirst and collectFirstSome to Foldable #2030

Conversation

kailuowang commented Nov 16, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rossabaker commented Nov 16, 2017

kailuowang commented Nov 16, 2017 • edited Loading

codecov-io commented Nov 16, 2017 • edited Loading

Codecov Report

rossabaker commented Nov 16, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kailuowang Nov 16, 2017 • edited Loading

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

johnynek commented Nov 17, 2017

kailuowang commented Nov 17, 2017

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

johnynek left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rossabaker left a comment

Choose a reason for hiding this comment

kailuowang commented Nov 20, 2017

rossabaker left a comment

Choose a reason for hiding this comment

added `collectFirst` and `collectFirstSome` to `Foldable` #2030

added `collectFirst` and `collectFirstSome` to `Foldable` #2030

kailuowang commented Nov 16, 2017 •

edited

Loading

codecov-io commented Nov 16, 2017 •

edited

Loading

kailuowang Nov 16, 2017 •

edited

Loading