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.

Sign up for GitHub

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

Jump to bottom

UnorderedFoldable#isEmpty default implementation is incorrect #2586

Merged
merged 3 commits into from
Oct 30, 2018
Merged

UnorderedFoldable#isEmpty default implementation is incorrect #2586

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
d0078c5
Fixed ieEmpty, added tests
barambani Oct 29, 2018
6bd09c2
prePR
barambani Oct 29, 2018
972db96
Changed iplementation
barambani Oct 30, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion core/src/main/scala/cats/UnorderedFoldable.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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))

Copy link
Member

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? :)

Copy link
Contributor Author

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 👍.

def nonEmpty[A](fa: F[A]): Boolean =
!isEmpty(fa)
Expand Down
62 changes: 58 additions & 4 deletions tests/src/test/scala/cats/tests/UnorderedFoldableSuite.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,24 +4,78 @@ package tests
import org.scalatest.prop.PropertyChecks
import org.scalacheck.Arbitrary
import cats.instances.all._
import cats.kernel.CommutativeMonoid

abstract class UnorderedFoldableSuite[F[_]: UnorderedFoldable](name: String)(implicit ArbFString: Arbitrary[F[String]])
sealed abstract class UnorderedFoldableSuite[F[_]](name: String)(implicit ArbFString: Arbitrary[F[String]])
extends CatsSuite
with PropertyChecks {

def iterator[T](fa: F[T]): Iterator[T]
def specializedUnorderedFoldMap[A, B: CommutativeMonoid](fa: F[A])(f: A => B): B

private[this] val instance: UnorderedFoldable[F] =
new UnorderedFoldable[F] {
def unorderedFoldMap[A, B: CommutativeMonoid](fa: F[A])(f: A => B): B =
specializedUnorderedFoldMap(fa)(f)
}

test(s"UnorderedFoldable[$name].isEmpty") {
forAll { fa: F[String] =>
instance.isEmpty(fa) should ===(instance.size(fa) === 0L)
}
}

test(s"UnorderedFoldable[$name].nonEmpty") {
forAll { fa: F[String] =>
instance.nonEmpty(fa) should ===(instance.size(fa) > 0L)
}
}

test(s"UnorderedFoldable[$name].count") {
forAll { (fa: F[String], p: String => Boolean) =>
fa.count(p) === iterator(fa).count(p).toLong
implicit val F: UnorderedFoldable[F] = instance
fa.count(p) should ===(iterator(fa).count(p).toLong)
}
}

test(s"UnorderedFoldable[$name].size") {
forAll { fa: F[String] =>
implicit val F: UnorderedFoldable[F] = instance
fa.count(Function.const(true)) should ===(fa.size)
}
}
}

final class UnorderedFoldableSetSuite extends UnorderedFoldableSuite[Set]("set") {
def iterator[T](set: Set[T]): Iterator[T] = set.iterator
def specializedUnorderedFoldMap[A, B: CommutativeMonoid](fa: Set[A])(f: A => B): B =
catsStdInstancesForSet.unorderedFoldMap(fa)(f)
}

final class UnorderedFoldableMapSuite extends UnorderedFoldableSuite[Map[String, ?]]("map") {
def iterator[T](map: Map[String, T]): Iterator[T] = map.valuesIterator
def specializedUnorderedFoldMap[A, B: CommutativeMonoid](fa: Map[String, A])(f: A => B): B =
catsStdInstancesForMap[String].unorderedFoldMap(fa)(f)
}

sealed abstract class SpecializedUnorderedFoldableSuite[F[_]: UnorderedFoldable](name: String)(
implicit ArbFString: Arbitrary[F[String]]
) extends CatsSuite
with PropertyChecks {

def iterator[T](fa: F[T]): Iterator[T]

test(s"Specialized UnorderedFoldable[$name].count") {
forAll { (fa: F[String], p: String => Boolean) =>
fa.count(p) should ===(iterator(fa).count(p).toLong)
}
}
}

class UnorderedFoldableSetSuite extends UnorderedFoldableSuite[Set]("set") {
final class SpecializedUnorderedFoldableSetSuite extends SpecializedUnorderedFoldableSuite[Set]("set") {
def iterator[T](set: Set[T]): Iterator[T] = set.iterator
}

class UnorderedFoldableMapSuite extends UnorderedFoldableSuite[Map[String, ?]]("map") {
final class SpecializedUnorderedFoldableMapSuite extends SpecializedUnorderedFoldableSuite[Map[String, ?]]("map") {
def iterator[T](map: Map[String, T]): Iterator[T] = map.valuesIterator
}