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

Removed deprecation of >> and changed its param to be a by-name #2043

Merged
merged 3 commits into from
Nov 26, 2017

Conversation

mpilquist
Copy link
Member

In FS2, we rely heavily on recursive functions that return values of type Pull[F,O,R]. These functions typically take a form similar to:

def foo: Pull[F,O,R] =
  if (guard) Pull.done else doSomeWork *> foo

Recently, @pchlupacek noticed that FS2 defines *> with a by-name param whereas cats defines *> with a strict param. Such recursive programs are broken if we use a strict param. Hence, we are going to change the FS2 definition of *> to be strict and reintroduce >> with a by-name param and then use >> in all recursive pulls.

Ideally, cats would have the same definition for >>, so in this PR I removed the deprecation of >> and defined it with a by-name param, making it a true alias for flatMap(_ => fb). Note that we already have followedByEval and forEffectEval, though the syntax overhead of using those is way too high IMO.

@LukaJCB
Copy link
Member

LukaJCB commented Nov 25, 2017

I'm generally +1 on this, but should we maybe move it to Apply?

@mpilquist
Copy link
Member Author

I thought about that but settled on leaving >> on FlatMap since the guarantee is that fb won't be computed until fa produces an A (precisely what flatMap describes).

@LukaJCB
Copy link
Member

LukaJCB commented Nov 25, 2017

I see, yeah I think that makes sense! :)

@LukaJCB
Copy link
Member

LukaJCB commented Nov 25, 2017

I think you'll need to add a mima exception

@mpilquist
Copy link
Member Author

Yeah, just pushed a mima fix.

@codecov-io
Copy link

codecov-io commented Nov 25, 2017

Codecov Report

Merging #2043 into master will increase coverage by 0.01%.
The diff coverage is 50%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2043      +/-   ##
==========================================
+ Coverage   95.05%   95.07%   +0.01%     
==========================================
  Files         311      311              
  Lines        5255     5255              
  Branches      114      124      +10     
==========================================
+ Hits         4995     4996       +1     
+ Misses        260      259       -1
Impacted Files Coverage Δ
core/src/main/scala/cats/FlatMap.scala 100% <ø> (ø) ⬆️
core/src/main/scala/cats/syntax/flatMap.scala 80% <50%> (+13.33%) ⬆️

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 bd940c5...ff5c3d8. Read the comment docs.

johnynek
johnynek previously approved these changes Nov 25, 2017
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.

This looks good to me.

@kailuowang kailuowang merged commit 563f612 into typelevel:master Nov 26, 2017
@kailuowang kailuowang added this to the 1.0.0 milestone Nov 26, 2017
This was referenced Dec 6, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants