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 Defer.fix #3208

Merged
merged 3 commits into from Dec 16, 2019
Merged

Add Defer.fix #3208

merged 3 commits into from Dec 16, 2019

Conversation

johnynek
Copy link
Contributor

A pattern I often use in working with scalacheck.Gen or with Parsers is:

lazy val x: Gen[X] = {
  val recurse = Gen.lzy(x)
  //....
}

It occurred to me all the types I use this pattern with are generally Defer instances, and more over, we can express fix nicely on Defer instances since we know we can always return a value (there is no risk of infinite loop at fix application, although it is up to the user if evaluating F[A] is safe.

I think this is a useful function to have on Defer.

}

forAll { n0: Int =>
val n = n0 & 0x3FF // don't let it get too big
Copy link
Contributor

Choose a reason for hiding this comment

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

This is nitpicky but could you just write this out for clarity? I can't remember off the top of my head what this does for negative n0 for example.

Copy link
Member

Choose a reason for hiding this comment

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

It's taking the 18 least significant bits. Perhaps a slightly clearer way to write it for those who aren't doing bit manipulation on a regular basis would be math.abs(n0 % 262144).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've changed it to be more direct Gen.choose(0, 1000) which I think is nicer anyway. Note 0x3ff = 1024, it is actually 8 + 2 bits.

@codecov-io
Copy link

codecov-io commented Dec 13, 2019

Codecov Report

Merging #3208 into master will increase coverage by 0.01%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3208      +/-   ##
==========================================
+ Coverage   93.05%   93.06%   +0.01%     
==========================================
  Files         376      376              
  Lines        7412     7428      +16     
  Branches      192      192              
==========================================
+ Hits         6897     6913      +16     
  Misses        515      515
Flag Coverage Δ
#scala_version_212 93.39% <ø> (+0.01%) ⬆️
#scala_version_213 92.84% <ø> (+0.01%) ⬆️
Impacted Files Coverage Δ
core/src/main/scala/cats/evidence/As.scala 100% <0%> (ø) ⬆️

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 67037ce...3398700. Read the comment docs.

djspiewak
djspiewak previously approved these changes Dec 13, 2019
def fix[A](fn: F[A] => F[A]): F[A] = {
lazy val res: F[A] = defer(fn(res))
res
}
Copy link
Member

Choose a reason for hiding this comment

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

This is rather nice! I never knew I wanted this.

LukaJCB
LukaJCB previously approved these changes Dec 13, 2019
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.

👍

kailuowang
kailuowang previously approved these changes Dec 13, 2019
Copy link
Contributor

@kailuowang kailuowang left a comment

Choose a reason for hiding this comment

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

nice! thanks!

@johnynek johnynek dismissed stale reviews from kailuowang, LukaJCB, and djspiewak via 3398700 December 14, 2019 01:31
@johnynek
Copy link
Contributor Author

One confusing aspect of cats contributions is that it is not clear when and why things are merged. e.g. two people have approved this PR without comment, but not merged. Is it waiting on a third or is there some other criteria that determines when a PR is merged?

@LukaJCB LukaJCB merged commit be23033 into master Dec 16, 2019
@LukaJCB LukaJCB deleted the oscar/defer_fix branch December 16, 2019 20:45
@LukaJCB
Copy link
Member

LukaJCB commented Dec 16, 2019

@johnynek sorry about that, I must have forgotten to press the button.

@travisbrown
Copy link
Contributor

@johnynek Sorry, probably partly my fault—I should have responded with a follow-up 👍. (My understanding is if there are two maintainer sign-offs and no outstanding requested changes you should feel free to merge yourself or ask for a merge.)

@johnynek
Copy link
Contributor Author

Okay, thanks for the merge. I wasn't sure if there was some reason for delay due to staging for releases.

@djspiewak
Copy link
Member

Okay, thanks for the merge. I wasn't sure if there was some reason for delay due to staging for releases.

In my experience, some of the reviewers just drive-by review things and don't loop back to check the merging status. This has made cats reviews a bit of a two phase process, where the second pass is just going through and pressing merge on a bunch of already-reviewed PRs.

Honestly I don't think this is entirely problematic so long as the maintainers are doing these phases frequently enough, but it is certainly confusing for contributors.

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.

None yet

6 participants