-
-
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
SemigroupK consistency and fix; added compose tests. #751
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -31,9 +31,9 @@ import simulacrum.{op, typeclass} | |
/** | ||
* Compose two SemigroupK intsances. | ||
*/ | ||
def composedWith[G[_]: SemigroupK]: SemigroupK[λ[α => F[G[α]]]] = | ||
def compose[G[_]: SemigroupK]: SemigroupK[λ[α => F[G[α]]]] = | ||
new SemigroupK[λ[α => F[G[α]]]] { | ||
def combine[A](x: F[G[A]], y: F[G[A]]): F[G[A]] = combine(x, y) | ||
def combine[A](x: F[G[A]], y: F[G[A]]): F[G[A]] = self.combine(x, y) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good catch! |
||
} | ||
|
||
/** | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,58 @@ | ||
package cats | ||
package tests | ||
|
||
import cats.data.{ NonEmptyList, NonEmptyVector, OneAnd } | ||
import cats.laws.discipline.{ ApplicativeTests, FoldableTests, MonoidalTests, SemigroupKTests, arbitrary, eq }, arbitrary._, eq._ | ||
import org.scalacheck.Arbitrary | ||
|
||
class ComposeTests extends CatsSuite { | ||
// we have a lot of generated lists of lists in these tests. We have to tell | ||
// Scalacheck to calm down a bit so we don't hit memory and test duration | ||
// issues. | ||
implicit override val generatorDrivenConfig: PropertyCheckConfiguration = | ||
PropertyCheckConfig(maxSize = 5, minSuccessful = 20) | ||
|
||
{ | ||
// Applicative composition | ||
|
||
implicit val applicativeListVector: Applicative[Lambda[A => List[Vector[A]]]] = Applicative[List] compose Applicative[Vector] | ||
implicit val iso = MonoidalTests.Isomorphisms.invariant[Lambda[A => List[Vector[A]]]] | ||
|
||
checkAll("Applicative[Lambda[A => List[Vector[A]]]]", ApplicativeTests[Lambda[A => List[Vector[A]]]].applicative[Int, Int, Int]) | ||
} | ||
|
||
{ | ||
// Foldable composition | ||
|
||
implicit val foldableListVector: Foldable[Lambda[A => List[Vector[A]]]] = Foldable[List] compose Foldable[Vector] | ||
|
||
checkAll("Foldable[Lambda[A => List[Vector[A]]]]", FoldableTests[Lambda[A => List[Vector[A]]]].foldable[Int, Int]) | ||
} | ||
|
||
{ | ||
// Reducible composition | ||
|
||
val nelReducible = | ||
new NonEmptyReducible[NonEmptyList, List] { | ||
def split[A](fa: NonEmptyList[A]): (A, List[A]) = (fa.head, fa.tail) | ||
} | ||
|
||
val nevReducible = | ||
new NonEmptyReducible[NonEmptyVector, Vector] { | ||
def split[A](fa: NonEmptyVector[A]): (A, Vector[A]) = (fa.head, fa.tail) | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I was surprised to see that these aren't already provided. I consider it outside of the scope of this PR, but I think we should follow up on this and add these instances in the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Agreed on both counts. |
||
|
||
implicit val reducibleListVector: Reducible[Lambda[A => NonEmptyList[NonEmptyVector[A]]]] = nelReducible compose nevReducible | ||
|
||
// No Reducible-specific laws, so check the Foldable laws are satisfied | ||
checkAll("Reducible[Lambda[A => List[Vector[A]]]]", FoldableTests[Lambda[A => NonEmptyList[NonEmptyVector[A]]]].foldable[Int, Int]) | ||
} | ||
|
||
{ | ||
// SemigroupK composition | ||
|
||
implicit val semigroupKListVector: SemigroupK[Lambda[A => List[Vector[A]]]] = SemigroupK[List] compose SemigroupK[Vector] | ||
|
||
checkAll("SemigroupK[Lambda[A => List[Vector[A]]]]", SemigroupKTests[Lambda[A => List[Vector[A]]]].semigroupK[Int]) | ||
} | ||
} |
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.
We have Alternative which extends both
Applicative
andMonoidK
. Not that calling itcomposedWith
is a particularly elegant solution, but do we run the risk of overloading ambiguities/annoyances with bothcompose
s being on the same instance?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.
I think the right thing to do here is to add a
compose
toAlternative
which composes a pair ofAlternatives
to anAlternative
. I can add that to this PR or follow up afterwards.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 sounds like a good solution to me. It might also mean we want to add something similar to
MonoidK
. I'm okay with that either going into this PR or a follow up one.