-
-
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
Adding get for Foldable #1464
Adding get for Foldable #1464
Changes from all commits
737e5de
d94f5e8
c829136
6dcea43
f093fb1
71d4fd6
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -117,6 +117,9 @@ private[data] sealed trait OneAndInstances extends OneAndLowPriority2 { | |
new NonEmptyReducible[OneAnd[F, ?], F] { | ||
override def split[A](fa: OneAnd[F, A]): (A, F[A]) = (fa.head, fa.tail) | ||
|
||
override def get[A](fa: OneAnd[F, A])(idx: Long): Option[A] = | ||
if (idx == 0L) Some(fa.head) else F.get(fa.tail)(idx - 1L) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we can add this implementation to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done. I know there isn't a test currently; but I think that if such a test exists it should be comparing the default implementation to any overrides which exist. Do you mind if we put that on a different issue because I think that the consistency of the methods is important. |
||
|
||
override def size[A](fa: OneAnd[F, A]): Long = 1 + F.size(fa.tail) | ||
} | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -70,6 +70,16 @@ trait ListInstances extends cats.kernel.instances.ListInstances { | |
G.map2Eval(f(a), lglb)(_ :: _) | ||
}.value | ||
|
||
@tailrec | ||
override def get[A](fa: List[A])(idx: Long): Option[A] = | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can I request we avoid fa match {
case Nil => None
case h :: tail =>
if (idx < 0) None
else if (idx == 0) h
else get(tail)(idx - 1)
} There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. done. |
||
fa match { | ||
case Nil => None | ||
case h :: tail => | ||
if (idx < 0) None | ||
else if (idx == 0) Some(h) | ||
else get(tail)(idx - 1) | ||
} | ||
|
||
override def exists[A](fa: List[A])(p: A => Boolean): Boolean = | ||
fa.exists(p) | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -6,16 +6,22 @@ import org.scalacheck.Arbitrary | |
import scala.util.Try | ||
|
||
import cats.instances.all._ | ||
import cats.data.{NonEmptyList, NonEmptyStream, NonEmptyVector, Validated} | ||
import cats.data._ | ||
import cats.laws.discipline.arbitrary._ | ||
|
||
abstract class FoldableCheck[F[_]: Foldable](name: String)(implicit ArbFInt: Arbitrary[F[Int]], ArbFString: Arbitrary[F[String]]) extends CatsSuite with PropertyChecks { | ||
|
||
def iterator[T](fa: F[T]): Iterator[T] | ||
|
||
test(s"Foldable[$name].size") { | ||
forAll { (fa: F[Int]) => | ||
fa.size should === (iterator(fa).size.toLong) | ||
test(s"Foldable[$name].size/get") { | ||
forAll { (fa: F[Int], n: Int) => | ||
val s = fa.size | ||
s should === (iterator(fa).size.toLong) | ||
if (n < s && n >= 0) { | ||
fa.get(n.toLong) === Some(iterator(fa).take(n + 1).toList.last) | ||
} else { | ||
fa.get(n.toLong) === None | ||
} | ||
} | ||
} | ||
|
||
|
@@ -217,7 +223,7 @@ class FoldableStreamCheck extends FoldableCheck[Stream]("stream") { | |
} | ||
|
||
class FoldableMapCheck extends FoldableCheck[Map[Int, ?]]("map") { | ||
def iterator[T](map: Map[Int, T]): Iterator[T] = map.iterator.map(_._2) | ||
def iterator[T](map: Map[Int, T]): Iterator[T] = map.valuesIterator | ||
} | ||
|
||
class FoldableOptionCheck extends FoldableCheck[Option]("option") { | ||
|
@@ -235,3 +241,38 @@ class FoldableValidatedCheck extends FoldableCheck[Validated[String, ?]]("valida | |
class FoldableTryCheck extends FoldableCheck[Try]("try") { | ||
def iterator[T](tryt: Try[T]): Iterator[T] = tryt.toOption.iterator | ||
} | ||
|
||
class FoldableEitherKCheck extends FoldableCheck[EitherK[Option, Option, ?]]("eitherK") { | ||
def iterator[T](eitherK: EitherK[Option, Option, T]) = eitherK.run.bimap(_.iterator, _.iterator).merge | ||
} | ||
|
||
class FoldableIorCheck extends FoldableCheck[Int Ior ?]("ior") { | ||
def iterator[T](ior: Int Ior T) = | ||
ior.fold(_ => None.iterator, b => Some(b).iterator, (_, b) => Some(b).iterator) | ||
} | ||
|
||
class FoldableIdCheck extends FoldableCheck[Id[?]]("id") { | ||
def iterator[T](id: Id[T]) = Some(id).iterator | ||
} | ||
|
||
class FoldableIdTCheck extends FoldableCheck[IdT[Option, ?]]("idT") { | ||
def iterator[T](idT: IdT[Option, T]) = idT.value.iterator | ||
} | ||
|
||
class FoldableConstCheck extends FoldableCheck[Const[Int, ?]]("const") { | ||
def iterator[T](const: Const[Int, T]) = None.iterator | ||
} | ||
|
||
class FoldableTuple2Check extends FoldableCheck[(Int, ?)]("tuple2") { | ||
def iterator[T](tuple: (Int, T)) = Some(tuple._2).iterator | ||
} | ||
|
||
class FoldableOneAndCheck extends FoldableCheck[OneAnd[List, ?]]("oneAnd") { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is sort of already checked through There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I agree there is some duplication, but I don't think it should be tackled in this issue and should probably a different issue where we discuss what to do with tests for a typeclass which aren't part of the discipline laws. |
||
def iterator[T](oneAnd: OneAnd[List, T]) = (oneAnd.head :: oneAnd.tail).iterator | ||
} | ||
|
||
class FoldableComposedCheck extends FoldableCheck[Nested[List, Option, ?]]("nested") { | ||
def iterator[T](nested: Nested[List, Option, T]) = nested.value.collect { | ||
case Some(t) => t | ||
}.iterator | ||
} |
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.
Shouldn't this be
else G.get(split(fa)._2)(idx - 1L)
?It's a bit funny that the only line of your PR that isn't covered by a unit test is the one with a bug in 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.
😢 You're right.
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.
Shit, looked over that one too quickly as well. Thanks @ceedubs for paying attention.