-
-
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
UnorderedFoldable#isEmpty default implementation is incorrect #2586
Conversation
Codecov Report
@@ Coverage Diff @@
## master #2586 +/- ##
==========================================
+ Coverage 95.14% 95.16% +0.01%
==========================================
Files 361 361
Lines 6634 6634
Branches 282 294 +12
==========================================
+ Hits 6312 6313 +1
+ Misses 322 321 -1
Continue to review full report at Codecov.
|
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 for the update of the tests!
👍 |
@@ -18,7 +18,7 @@ import cats.instances.long._ | |||
* Returns true if there are no elements. Otherwise false. | |||
*/ | |||
def isEmpty[A](fa: F[A]): Boolean = | |||
exists(fa)(Function.const(true)) | |||
!exists(fa)(Function.const(true)) | |||
|
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.
Tiny tiny nitpick but nonEmpty
now negates exists
twice. Maybe we should define nonEmpty
as exists(fa)(Function.const(true))
and then isEmpty
as !nonEmpty
WDYT? :)
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.
Actually I started from that :), but then I changed it as I had the impression it was reading better in my head (not exists instead of not non empty). But I don't really love my version either, as you said double negates, and even if it's simple logic, that makes reasoning convoluted and unnatural. I'm happy to change it 👍.
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.
Thanks a ton for fixing this so quickly! :)
thank you for the great help. As usual 👍 |
Fixes issue #2584 and adds some tests.