-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Add Defer.fix #3208
Conversation
} | ||
|
||
forAll { n0: Int => | ||
val n = n0 & 0x3FF // don't let it get too big |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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)
.
There was a problem hiding this comment.
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 Report
@@ 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
Continue to review full report at Codecov.
|
def fix[A](fn: F[A] => F[A]): F[A] = { | ||
lazy val res: F[A] = defer(fn(res)) | ||
res | ||
} |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice! thanks!
3398700
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? |
@johnynek sorry about that, I must have forgotten to press the button. |
@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.) |
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. |
A pattern I often use in working with scalacheck.Gen or with Parsers is:
It occurred to me all the types I use this pattern with are generally Defer instances, and more over, we can express
fix
nicely onDefer
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 evaluatingF[A]
is safe.I think this is a useful function to have on Defer.