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.

Sign up for GitHub

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

Jump to bottom

Add SemigroupK instance for Xor + tests #996

Merged
merged 5 commits into from
Apr 27, 2016
+21 −35
Merged

Add SemigroupK instance for Xor + tests #996

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
63d186b
Add SemigroupK instance for Xor + tests
Apr 24, 2016
e66d238
XorT: remove MonoidK inst. + new SemigroupK inst.
Apr 26, 2016
0bae0ca
Xor: SemigroupK w/ first-right,last-left semantics
Apr 26, 2016
8636343
Remove implct Semigroup from Xor{T} SemigroupK tests
Apr 26, 2016
db83772
Move Xor{T} SemigroupK tests into broader scope
Apr 26, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions core/src/main/scala/cats/data/Xor.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -168,6 +168,14 @@ private[data] sealed abstract class XorInstances extends XorInstances1 {
def combine(x: A Xor B, y: A Xor B): A Xor B = x combine y
}

implicit def xorSemigroupK[L]: SemigroupK[Xor[L,?]] =
new SemigroupK[Xor[L,?]] {
def combineK[A](x: Xor[L,A], y: Xor[L,A]): Xor[L,A] = x match {
case Xor.Left(_) => y
case Xor.Right(_) => x
}
}

implicit def xorBifunctor: Bitraverse[Xor] =
new Bitraverse[Xor] {
def bitraverse[G[_], A, B, C, D](fab: Xor[A, B])(f: A => G[C], g: B => G[D])(implicit G: Applicative[G]): G[Xor[C, D]] =
Expand Down
34 changes: 7 additions & 27 deletions core/src/main/scala/cats/data/XorT.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -215,16 +215,6 @@ private[data] abstract class XorTInstances1 extends XorTInstances2 {
}
*/

/* TODO delete this when MonadCombine instance is re-enabled */
implicit def xorTMonoidK[F[_], L](implicit F: Monad[F], L: Monoid[L]): MonoidK[XorT[F, L, ?]] = {
implicit val F0 = F
implicit val L0 = L
new MonoidK[XorT[F, L, ?]] with XorTSemigroupK[F, L] {
implicit val F = F0; implicit val L = L0
def empty[A]: XorT[F, L, A] = XorT.left(F.pure(L.empty))(F)
}
}

implicit def xorTFoldable[F[_], L](implicit F: Foldable[F]): Foldable[XorT[F, L, ?]] =
new XorTFoldable[F, L] {
val F0: Foldable[F] = F
Expand All @@ -242,10 +232,13 @@ private[data] abstract class XorTInstances2 extends XorTInstances3 {
new XorTMonadError[F, L] { implicit val F = F0 }
}

implicit def xorTSemigroupK[F[_], L](implicit F: Monad[F], L: Semigroup[L]): SemigroupK[XorT[F, L, ?]] = {
implicit val F0 = F
implicit val L0 = L
new XorTSemigroupK[F, L] { implicit val F = F0; implicit val L = L0 }
implicit def xorTSemigroupK[F[_], L](implicit F: Monad[F]): SemigroupK[XorT[F, L, ?]] =
new SemigroupK[XorT[F,L,?]] {
def combineK[A](x: XorT[F,L,A], y: XorT[F, L, A]): XorT[F, L, A] =
XorT(F.flatMap(x.value) {
case l @ Xor.Left(_) => y.value
case r @ Xor.Right(_) => F.pure(r)
})
}

implicit def xorTEq[F[_], L, R](implicit F: Eq[F[L Xor R]]): Eq[XorT[F, L, R]] =
Expand Down Expand Up @@ -288,19 +281,6 @@ private[data] trait XorTMonadError[F[_], L] extends MonadError[XorT[F, L, ?], L]
fla.recoverWith(pf)
}

private[data] trait XorTSemigroupK[F[_], L] extends SemigroupK[XorT[F, L, ?]] {
implicit val F: Monad[F]
implicit val L: Semigroup[L]
def combineK[A](x: XorT[F, L, A], y: XorT[F, L, A]): XorT[F, L, A] =
XorT(F.flatMap(x.value) {
case Xor.Left(l1) => F.map(y.value) {
case Xor.Left(l2) => Xor.Left(L.combine(l1, l2))
case r @ Xor.Right(_) => r
}
case r @ Xor.Right(_) => F.pure[L Xor A](r)
})
}

private[data] trait XorTMonadFilter[F[_], L] extends MonadFilter[XorT[F, L, ?]] with XorTMonadError[F, L] {
implicit val F: Monad[F]
implicit val L: Monoid[L]
Expand Down
2 changes: 0 additions & 2 deletions tests/src/test/scala/cats/tests/XorTTests.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,8 +14,6 @@ class XorTTests extends CatsSuite {
implicit val iso = CartesianTests.Isomorphisms.invariant[XorT[List, String, ?]]
checkAll("XorT[List, String, Int]", MonadErrorTests[XorT[List, String, ?], String].monadError[Int, Int, Int])
checkAll("MonadError[XorT[List, ?, ?]]", SerializableTests.serializable(MonadError[XorT[List, String, ?], String]))
checkAll("XorT[List, String, Int]", MonoidKTests[XorT[List, String, ?]].monoidK[Int])
checkAll("MonoidK[XorT[List, String, ?]]", SerializableTests.serializable(MonoidK[XorT[List, String, ?]]))
checkAll("XorT[List, ?, ?]", BifunctorTests[XorT[List, ?, ?]].bifunctor[Int, Int, Int, String, String, String])
checkAll("Bifunctor[XorT[List, ?, ?]]", SerializableTests.serializable(Bifunctor[XorT[List, ?, ?]]))
Copy link
Contributor

Choose a reason for hiding this comment

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

This is minor, but now that the SemigroupK instance no longer requires a Semigroup[L], the SemigroupK tests down at line ~49 don't need the local Semigroup[ListWrapper[String]] instance and could be moved up here, similarly to this comment.

checkAll("XorT[List, Int, ?]", TraverseTests[XorT[List, Int, ?]].traverse[Int, Int, Int, Int, Option, Option])
Expand Down
7 changes: 7 additions & 0 deletions tests/src/test/scala/cats/tests/XorTests.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ package tests

import cats.data.{NonEmptyList, Xor, XorT}
import cats.data.Xor._
import cats.laws.discipline.{SemigroupKTests}
import cats.laws.discipline.arbitrary._
import cats.laws.discipline.{BitraverseTests, TraverseTests, MonadErrorTests, SerializableTests, CartesianTests}
import cats.laws.discipline.eq.tuple3Eq
Expand Down Expand Up @@ -46,6 +47,12 @@ class XorTests extends CatsSuite {
checkAll("Eq[ListWrapper[String] Xor ListWrapper[Int]]", SerializableTests.serializable(Eq[ListWrapper[String] Xor ListWrapper[Int]]))
}

{
implicit val L = ListWrapper.semigroup[String]
Copy link
Contributor

Choose a reason for hiding this comment

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

This line is creating a local (to the surrounding { ... } block) Semigroup[ListWrapper[String]] instance. We are taking this local instance approach in some tests, because we want to make sure that things work given the minimum required instances. For example, does this work when we have a Semigroup[ListWrapper[String]] but no Monoid[ListWrapper[String]]?

In this case, the SemigroupK instance you created doesn't have any requirements (specifically, no Semigroup[L] instance is required). So we shouldn't have to take this approach here. We should be able to just add the 2 checkAll tests around line 34 without creating a separate block to house locally-scoped instances.

I'd imagine all of these locally-scoped instances are somewhat terrifying for someone who is used to Haskell :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah, good call. this is what I get for submitting a PR at 8am before breakfast :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

and, yeah, coming from Haskell it feels like these instances are just flying around and I have no idea what's happening :) For example, I didn't know you could introduce a local instance by wrapping something in curly braces.

Copy link
Contributor

@ceedubs ceedubs Apr 26, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

Yeah, the implicit scope rules are incredibly complicated (which admittedly we use as a "feature" quite a bit in cats to aid in implicit resolution). In this case we are using the curly braces to have it scoped locally to that block instead of to the whole class/object.

😈

checkAll("Xor[ListWrapper[String], ?]", SemigroupKTests[Xor[ListWrapper[String], ?]].semigroupK[Int])
checkAll("SemigroupK[Xor[ListWrapper[String], ?]]", SerializableTests.serializable(SemigroupK[Xor[ListWrapper[String], ?]]))
}

implicit val arbitraryXor: Arbitrary[Xor[Int, String]] = Arbitrary {
for {
left <- arbitrary[Boolean]
Expand Down