-
-
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
Override Foldable methods #1532
Conversation
Codecov Report@@ Coverage Diff @@
## master #1532 +/- ##
==========================================
+ Coverage 92.26% 92.48% +0.21%
==========================================
Files 246 246
Lines 3764 3885 +121
Branches 125 133 +8
==========================================
+ Hits 3473 3593 +120
- Misses 291 292 +1
Continue to review full report at Codecov.
|
- Override Foldable methods in a lot of instances (including NonEmptyReducible) - Override MonoidK algebra to get kernel Monoid instance - Add Reducible for Tuple2 I might have gone overboard with the "one element foldables" (Option, Either, ...).
8549619
to
0f6775d
Compare
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.
that extension shows you what is untested. Seems like we need to enhance the laws here.
I really like this PR though, since it affords some chances for optimizations, which is nice.
a :: G.toList(ga) | ||
} | ||
|
||
override def toNonEmptyList[A](fa: F[A]): NonEmptyList[A] = { |
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 method code path is untested
fa.reduce | ||
|
||
override def foldM[G[_], A, B](fa: NonEmptyVector[A], z: B)(f: (B, A) => G[B])(implicit G: Monad[G]): G[B] = | ||
Foldable.iteratorFoldM(fa.toVector.toIterator, z)(f) |
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.
code path is untested.
@@ -323,7 +324,39 @@ private[data] sealed abstract class ValidatedInstances extends ValidatedInstance | |||
case v @ Validated.Valid(_) => v | |||
} | |||
def raiseError[A](e: E): Validated[E, A] = Validated.Invalid(e) | |||
|
|||
override def reduceLeftToOption[A, B](fa: Validated[E, A])(f: A => B)(g: (B, A) => B): Option[B] = |
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.
next 5 methods untested.
fa.forall(p) | ||
|
||
override def toList[A](fa: Validated[E, A]): List[A] = | ||
fa.fold(_ => Nil, _ :: Nil) |
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.
untested
override def ensure[B](fab: Either[A, B])(error: => A)(predicate: B => Boolean): Either[A, B] = | ||
fab.ensure(error)(predicate) | ||
|
||
override def reduceLeftToOption[B, C](fab: Either[A, B])(f: B => C)(g: (C, B) => C): Option[C] = |
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.
next 5 method untested.
fab.right.forall(p) | ||
|
||
override def toList[B](fab: Either[A, B]): List[B] = | ||
fab.fold(_ => Nil, _ :: Nil) |
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.
untested.
@@ -71,12 +71,46 @@ trait OptionInstances extends cats.kernel.instances.OptionInstances { | |||
override def filter[A](fa: Option[A])(p: A => Boolean): Option[A] = | |||
fa.filter(p) | |||
|
|||
override def reduceLeftToOption[A, B](fa: Option[A])(f: A => B)(g: (B, A) => B): Option[B] = |
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.
next 5 methods untested.
override def exists[A](fa: Option[A])(p: A => Boolean): Boolean = | ||
fa.exists(p) | ||
|
||
override def forall[A](fa: Option[A])(p: A => Boolean): Boolean = | ||
fa.forall(p) | ||
|
||
override def toList[A](fa: Option[A]): List[A] = fa.toList | ||
|
||
override def filter_[A](fa: Option[A])(p: A => Boolean): List[A] = |
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.
next 5 methods untested.
04bbbdd
to
75580ba
Compare
Improved the test coverage, only the methods |
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 great. Thanks for beefing up the tests and optimizing a bit!
I think the code is reasonable, especially now that we have full test coverage. |
I concur that some benchmarking would be nice to make these performance/complexity decisions. |
merging with two thumb ups. |
Foldable
methods in a lot of instances (includingReducible
andNonEmptyReducible
)MonoidK#algebra
forList
,Vector
, etc to get kernelMonoid
instanceReducible
forTuple2
I might have gone a bit overboard with overriding in the "one element
Foldable
" instances (Option
,Either
, ...).