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.

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

Merged
merged 3 commits into from
Feb 28, 2017
Merged

Conversation

peterneyens
Copy link
Collaborator

  • Override Foldable methods in a lot of instances (including Reducible and NonEmptyReducible)
  • Override MonoidK#algebra for List, Vector, etc to get kernel Monoid instance
  • Add Reducible for Tuple2

I might have gone a bit overboard with overriding in the "one element Foldable" instances (Option, Either, ...).

@codecov-io
Copy link

codecov-io commented Feb 2, 2017

Codecov Report

Merging #1532 into master will increase coverage by 0.21%.

@@            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
Impacted Files Coverage Δ
...in/scala/cats/laws/discipline/ReducibleTests.scala 100% <ø> (ø)
core/src/main/scala/cats/Foldable.scala 93.84% <ø> (-4.62%)
...ain/scala/cats/laws/discipline/TraverseTests.scala 100% <ø> (ø)
...ala/cats/laws/discipline/TraverseFilterTests.scala 100% <ø> (ø)
core/src/main/scala/cats/Reducible.scala 94.82% <100%> (+4.5%)
core/src/main/scala/cats/instances/try.scala 85.18% <100%> (+6.23%)
...ain/scala/cats/laws/discipline/FoldableTests.scala 100% <100%> (ø)
core/src/main/scala/cats/data/Validated.scala 100% <100%> (ø)
core/src/main/scala/cats/package.scala 100% <100%> (+4%)
core/src/main/scala/cats/instances/vector.scala 97.72% <100%> (+0.16%)
... and 12 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7919a30...96c6246. Read the comment docs.

- 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, ...).
Copy link
Contributor

@johnynek johnynek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

check out: https://chrome.google.com/webstore/detail/codecov-extension/keefkhehidemnokodkdkejapdgfjmijf?hl=en-US

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] = {
Copy link
Contributor

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)
Copy link
Contributor

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] =
Copy link
Contributor

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)
Copy link
Contributor

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] =
Copy link
Contributor

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)
Copy link
Contributor

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] =
Copy link
Contributor

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] =
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

next 5 methods untested.

@peterneyens peterneyens changed the title Override Foldable methods Override Foldable methods (wip) Feb 2, 2017
@peterneyens
Copy link
Collaborator Author

Improved the test coverage, only the methods reduceLeftToOption and reduceRightToOption are not yet tested.

Copy link
Contributor

@johnynek johnynek left a 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!

@kailuowang
Copy link
Contributor

I think the code is reasonable, especially now that we have full test coverage.
I just want to share my thoughts. To me, whether to override a default implementation provided by the type class is a trade-off between performance and complexity. The complexity cost comes in in the sense of extra code to maintain and test against, and in some cases, extra laws. Very often the performance gain outweighs the complexity cost. I feel that it's a pity we don't have a really good gauge on some of the performance gain here, e.g. the one element Foldables. On the other hand, the complexity cost isn't very substantial either.
For now, I think this PR is good for my 👍.
Hopefully, in the future, we'll have some benchmarking to give us more data when making such performance/complexity trade-off decisions.

@peterneyens peterneyens changed the title Override Foldable methods (wip) Override Foldable methods Feb 3, 2017
@peterneyens
Copy link
Collaborator Author

I concur that some benchmarking would be nice to make these performance/complexity decisions.

@kailuowang
Copy link
Contributor

merging with two thumb ups.

@kailuowang kailuowang merged commit a0c6f59 into typelevel:master Feb 28, 2017
@peterneyens peterneyens deleted the override-foldable branch April 23, 2017 23:49
@kailuowang kailuowang modified the milestone: 1.0.0-MF Apr 26, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants