Skip to content
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/fix Foldable extensions: findM and collectFirstSomeM #2421

Merged
merged 4 commits into from
Aug 28, 2018

Conversation

catostrophe
Copy link
Contributor

#2365 seems abandoned, so this is a rework of findM implemented as a syntax extension for binary compatibility. Another but equivalent implementation chosen, because Foldable.Source is private.

@codecov-io
Copy link

codecov-io commented Aug 18, 2018

Codecov Report

Merging #2421 into master will increase coverage by 0.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2421      +/-   ##
==========================================
+ Coverage    95.2%   95.22%   +0.01%     
==========================================
  Files         351      351              
  Lines        6366     6371       +5     
  Branches      280      279       -1     
==========================================
+ Hits         6061     6067       +6     
+ Misses        305      304       -1
Impacted Files Coverage Δ
core/src/main/scala/cats/Foldable.scala 100% <ø> (ø) ⬆️
core/src/main/scala/cats/syntax/foldable.scala 100% <100%> (ø) ⬆️
...rc/main/scala/cats/laws/discipline/Arbitrary.scala 99.17% <0%> (+0.07%) ⬆️
core/src/main/scala/cats/data/Chain.scala 97.42% <0%> (+0.32%) ⬆️

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 d878c49...ce25400. Read the comment docs.

LukaJCB
LukaJCB previously approved these changes Aug 18, 2018
Copy link
Member

@LukaJCB LukaJCB left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work, thank you :)

def findM[G[_]](p: A => G[Boolean])(implicit F: Foldable[F], G: Monad[G]): G[Option[A]] =
F.foldRight(fa, Eval.now(G.pure(Option.empty[A])))((a, lb) =>
Eval.now(G.flatMap(p(a))(if (_) G.pure(Some(a)) else lb.value))
).value
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I’m worried about stack safety here. Note findM, foldM and existsM use tailRecM to be stack safe.

Can we make Source package private so that we can use it here with tailRecM?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point. I assumed stack-safety is provided by safe flatMap implementation. Will rework via tailRecM.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also added stack-safety tests for other monadic folds.

Fix implementation for findM and collectFirstSomeM extensions for Foldable vai tailRecM, make Foldable.Source package private
@catostrophe catostrophe changed the title Add findM to Foldable extensions Add/fix Foldable extensions: findM and collectFirstSomeM Aug 18, 2018
@catostrophe
Copy link
Contributor Author

Can anything else be improved here to give it the green light?

Copy link
Member

@LukaJCB LukaJCB left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me, thank you! Excellent PR

Copy link
Contributor

@johnynek johnynek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

minor comment.

* }}}
*/
def findM[G[_]](p: A => G[Boolean])(implicit F: Foldable[F], G: Monad[G]): G[Option[A]] =
G.tailRecM(Foldable.Source.fromFoldable(fa))(_.uncons match {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

couldn't this be collectFirstSome { a => p(a).map(if (_) Some(a) else None) }?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure it could but would require an extra map for each iteration. I just followed the recommendation of other contributors to prefer efficiency over conciseness in code.

@LukaJCB
Copy link
Member

LukaJCB commented Aug 28, 2018

Shall we merge this, then? :)

Copy link
Member

@rossabaker rossabaker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@LukaJCB LukaJCB merged commit d10bddf into typelevel:master Aug 28, 2018
@LukaJCB LukaJCB added this to the 1.3 milestone Aug 28, 2018
@KentShikama
Copy link

@catostrophe Thanks for cleaning up after me

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.

6 participants