Fix issue in Reducible

[yilinwei]

courtesy of @ceedubs finding the bug.

[ceedubs]

👍 LGTM but maybe this is a sign that we should have a unit test for this :)

[yilinwei]

@ceedubs point taken....

[codecov-io]

Codecov Report

Merging #1685 into master will increase coverage by 0.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #1685      +/-   ##
==========================================
+ Coverage   93.94%   93.96%   +0.01%     
==========================================
  Files         241      241              
  Lines        4096     4091       -5     
  Branches      156      151       -5     
==========================================
- Hits         3848     3844       -4     
+ Misses        248      247       -1

Impacted Files	Coverage Δ
core/src/main/scala/cats/Reducible.scala	94.91% <100%> (+1.69%)	⬆️
core/src/main/scala/cats/Eval.scala	97.1% <0%> (-0.2%)	⬇️
core/src/main/scala/cats/data/EitherT.scala	96.52% <0%> (ø)	⬆️

---

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 cc4080c...bb9f8c9. Read the comment docs.

[kailuowang]

I think in principle every method that is not a simple delegation should be covered. This chrome extension is really helpful in checking that.

[peterneyens]

I think the problem here is that all the instances of Reducible override get. Another method I can think of with the same issue is Foldable#toList / Reducible#toNonEmptyList.

It would be nice if we could somehow check the consistency (#1684) and benchmark (something we talked about in #1532, see #1532 (comment)) overridden and the default implementation of type class methods.

[kailuowang]

How about we add a doctest to cover this and address the overrides test coverage in #1684? That way we can merge this fix sooner.

[peterneyens]

@kailuowang, do you mean something like :

scala> import cats.data.NonEmptyList
scala> import cats.implicits._

scala> val R = new NonEmptyReducible[NonEmptyList, List] {
     |   def split[A](nel: NonEmptyList[A]): (A, List[A]) = (nel.head, nel.tail)
     | }

scala> val nel = NonEmptyList.of(1,2,3)

scala> R.get(nel, 1L)
res0: Option[Int] = Some(2)

scala> R.get(nel, 4L)
res1: Option[Int] = None

[kailuowang]

@peterneyens yup. that would work.

[johnynek]

this is not actually tested according to codecov.

Can we write a test to explicitly exercise this?

[yilinwei]

@johnynek done.

[yilinwei]

Failure in the test is due to the Kleisli Eq presumably not working as intended; given that this PR doesn't touch that are we good to merge?

[djspiewak]

Merging with two sign-offs.