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

Fix many cats-kernel instances. #1324

Merged
merged 7 commits into from
Aug 24, 2016
Merged

Conversation

non
Copy link
Contributor

@non non commented Aug 23, 2016

This commit does a bunch of things:

  1. Move relevant either/function instnaces into kernel
  2. Add missing Eq/PartialOrder instances for Either
  3. Add many missing instances for Function0/Function1
  4. Add instances for BitSet
  5. Remove code duplication between collection monoids
  6. Improve test coverage of function instances
  7. Add some missing package objects in kernel
  8. A few stylistic changes

This is a relatively large commit but I think almost all of this is
stuff we'd like to get done before we freeze cats-kernel."

This fixes #1320.

This commit does a bunch of things:

 1. Move relevant either/function instnaces into kernel
 2. Add missing Eq/PartialOrder instances for Either
 3. Add many missing instances for Function0/Function1
 4. Add instances for BitSet
 5. Remove code duplication between collection monoids
 6. Improve test coverage of function instances
 7. Add some missing package objects in kernel
 8. A few stylistic changes

This is a relatively large commit but I think almost all of this is
stuff we'd like to get done before we freeze cats-kernel.
@non
Copy link
Contributor Author

non commented Aug 23, 2016

(Another change I failed to mention was that I made several of the Show instances more efficient.)

@non
Copy link
Contributor Author

non commented Aug 23, 2016

Review by @johnynek, plus anyone else who is interested.

@non
Copy link
Contributor Author

non commented Aug 23, 2016

We may want to start using things Helpers._ in our other tests -- it makes it a lot easier to be sure you're exercising all the different possible type class constructors.

@codecov-io
Copy link

codecov-io commented Aug 23, 2016

Current coverage is 91.55% (diff: 97.80%)

Merging #1324 into master will increase coverage by 0.07%

@@             master      #1324   diff @@
==========================================
  Files           234        237     +3   
  Lines          3543       3563    +20   
  Methods        3484       3504    +20   
  Messages          0          0          
  Branches         59         59          
==========================================
+ Hits           3241       3262    +21   
+ Misses          302        301     -1   
  Partials          0          0          

Sunburst

Powered by Codecov. Last update 06539fa...24e4de2

@kailuowang
Copy link
Contributor

👍 looks great to me. I wish we had that Helper before. Thanks!

b => y.fold(_ => 1, B.partialCompare(b, _))
implicit def catsStdShowForEither[A, B](implicit A: Show[A], B: Show[B]): Show[Either[A, B]] =
new Show[Either[A, B]] {
def show(f: Either[A, B]): String = f.fold(
Copy link
Contributor

Choose a reason for hiding this comment

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

we have biased to match for alleged performance reasons. What do you think of it here?

@@ -61,7 +61,7 @@ trait EitherInstances extends cats.kernel.instances.EitherInstances {

def traverse[F[_], B, C](fa: Either[A, B])(f: B => F[C])(implicit F: Applicative[F]): F[Either[A, C]] =
fa match {
case left @ Left(_) => F.pure(left)
case Left(a) => F.pure(Left(a))
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe: https://github.com/typelevel/cats/blob/master/core/src/main/scala/cats/syntax/either.scala#L301

case left @ Left(_) => F.pure(left.rightCast[C])

save an allocation?

@@ -48,7 +48,7 @@ trait EitherInstances extends cats.kernel.instances.EitherInstances {
@tailrec
def tailRecM[B, C](b: B)(f: B => Either[A, Either[B, C]]): Either[A, C] =
f(b) match {
case Left(a) => Left(a)
case left @ Left(_) => left.rightCast[C]
case Right(Left(b1)) => tailRecM(b1)(f)
case Right(Right(c)) => Right(c)
Copy link
Contributor

Choose a reason for hiding this comment

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

case Right(r) => r.leftCast[A] should work, right?

@johnynek
Copy link
Contributor

one last allocation to save, then I'm +1

@non
Copy link
Contributor Author

non commented Aug 23, 2016

@johnynek I think this is it right?

@johnynek
Copy link
Contributor

👍 Thank you for this work!

@non non merged commit 3ca2309 into typelevel:master Aug 24, 2016
@non non removed the in progress label Aug 24, 2016
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.

Move relevant function instances into kernel
5 participants