Override Foldable methods by peterneyens · Pull Request #1532 · typelevel/cats

peterneyens · 2017-02-02T17:43:17Z

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 · 2017-02-02T18:26:22Z

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, ...).

johnynek

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.

johnynek · 2017-02-02T19:06:50Z

+    a :: G.toList(ga)
+  }
+
+  override def toNonEmptyList[A](fa: F[A]): NonEmptyList[A] = {


this method code path is untested

johnynek · 2017-02-02T19:09:00Z

+        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)


code path is untested.

johnynek · 2017-02-02T19:09:44Z

        }
      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] =


next 5 methods untested.

johnynek · 2017-02-02T19:10:36Z

+        fa.forall(p)
+
+      override def toList[A](fa: Validated[E, A]): List[A] =
+        fa.fold(_ => Nil, _ :: Nil)


johnynek · 2017-02-02T19:10:49Z

      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] =


next 5 method untested.

johnynek · 2017-02-02T19:10:57Z

+        fab.right.forall(p)
+
+      override def toList[B](fab: Either[A, B]): List[B] =
+        fab.fold(_ => Nil, _ :: Nil)


johnynek · 2017-02-02T19:12:50Z

      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] =


next 5 methods untested.

johnynek · 2017-02-02T19:13:06Z


+      override def toList[A](fa: Option[A]): List[A] = fa.toList
+
+      override def filter_[A](fa: Option[A])(p: A => Boolean): List[A] =


next 5 methods untested.

peterneyens · 2017-02-03T01:15:23Z

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

johnynek

This is great. Thanks for beefing up the tests and optimizing a bit!

kailuowang · 2017-02-03T18:36:58Z

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 · 2017-02-03T18:50:26Z

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

kailuowang · 2017-02-28T19:00:44Z

merging with two thumb ups.

stew added the in progress label Feb 2, 2017

Override Foldable methods

0f6775d

- 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, ...).

peterneyens force-pushed the override-foldable branch from 8549619 to 0f6775d Compare February 2, 2017 19:09

johnynek requested changes Feb 2, 2017

View reviewed changes

peterneyens changed the title ~~Override Foldable methods~~ Override Foldable methods (wip) Feb 2, 2017

Improve test coverage overridden Foldable methods

75580ba

peterneyens force-pushed the override-foldable branch from 04bbbdd to 75580ba Compare February 3, 2017 00:23

Add reduceLeft/RightOption consistency laws

96c6246

johnynek approved these changes Feb 3, 2017

View reviewed changes

peterneyens changed the title ~~Override Foldable methods (wip)~~ Override Foldable methods Feb 3, 2017

kailuowang merged commit a0c6f59 into typelevel:master Feb 28, 2017

kailuowang removed the in progress label Feb 28, 2017

peterneyens deleted the override-foldable branch April 23, 2017 23:49

kailuowang modified the milestone: 1.0.0-MF Apr 26, 2017

This was referenced May 19, 2017

Fix issue in Reducible #1685

Merged

Consistency tests for override instances #1684

Open


		override def toList[A](fa: Option[A]): List[A] = fa.toList

		override def filter_[A](fa: Option[A])(p: A => Boolean): List[A] =

Uh oh!

Conversation

peterneyens commented Feb 2, 2017

Uh oh!

codecov-io commented Feb 2, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

johnynek left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

peterneyens commented Feb 3, 2017

Uh oh!

johnynek left a comment

Choose a reason for hiding this comment

Uh oh!

kailuowang commented Feb 3, 2017

Uh oh!

peterneyens commented Feb 3, 2017

Uh oh!

kailuowang commented Feb 28, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

codecov-io commented Feb 2, 2017 •

edited

Loading