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

Confusing behavior of Monoid[Xor[A, B]]#combine #888

Closed
notxcain opened this issue Feb 18, 2016 · 2 comments
Closed

Confusing behavior of Monoid[Xor[A, B]]#combine #888

notxcain opened this issue Feb 18, 2016 · 2 comments
Assignees
Milestone

Comments

@notxcain
Copy link
Contributor

Don’t you feel that it’s strange that Monoid[Xor[A, B]] requires A: Semigroup?
I expected combine to be implemented like this

def combine(x: A Xor B, y: A Xor B): A Xor B =
  x.flatMap { xb =>
    y.map { yb => Monoid[B].combine(xb, yb) }
  }

In current implementation it also tries to combine Lefts

@benhutchison
Copy link
Member

It seems that two lefts combine, and two right combine, but a left and a right combined is a left. I think that is consistent with Scalaz, eg [https://gist.github.com/travisbrown/3732844]. It's an interpretation where a failure of a part fails the combined group.

But I can definitely imagine cases where you'd want to opposite, to discard failures. Like a lot of types, there are several monoids for Xor and one gets blessed as the default. But IMO its a mistake to think of it as the monoid for Xor.

Welcome to the Ammonite Repl 0.5.2
(Scala 2.11.7 Java 1.8.0_51)
@ load.ivy("org.typelevel" %% "cats" % "0.4.1")
@ import cats._
import cats._
@ import cats.data._
import cats.data._
@ import cats.syntax.all._
import cats.syntax.all._
@ val left1 = Xor.left[String, Int]("fail1")
left1: Xor[String, Int] = Left(fail1)
@ val right = Xor.right[String, Int](7)
right: Xor[String, Int] = Right(7)
@ import cats.std.all._
import cats.std.all._
@ left1 |+| left1
res7: Xor[String, Int] = Left(fail1fail1)
@ left1 |+| right
res8: Xor[String, Int] = Left(fail1)
@ right |+| right
res9: Xor[String, Int] = Right(14)

@ceedubs
Copy link
Contributor

ceedubs commented May 6, 2016

@notxcain sorry I missed this issue when you first opened it, but I was thinking the same thing the other day. I would expect the errors to combine for Validated but not for Xor.

There was a conversation about this on Gitter yesterday.

@ceedubs ceedubs added this to the cats-1.0.0 milestone May 6, 2016
ceedubs added a commit to ceedubs/cats that referenced this issue May 14, 2016
Resolves typelevel#888

I don't think there's really a _right_ answer here. Both methods of
combining `Xor`s are straightforward and law-abiding. However, I think
that since in pretty much every other context (including the
`SemigroupK` instances), `Xor` does not combine failures and `Validated`
does, it is less surprising behavior to not combine left values in
`combine` methods for `Xor` and `XorT`.

Some notes about related implementations:
- Scalaz's respective methods (and semigroup instances) _do_ combine
  errors, so this is a deviation from that.
- The `Semigroup` for `Either` in Haskell has doesn't combine values at
  all, but returns the first `Right` (if any), making it equivalent to
  the behavior of `orElse` and the `SemigroupK` instance for `Xor`.
  Since we have already decided to not go with `orElse`-like behavior
  for our `Option` semigroup, I'm inclined to not take this approach for
  `Xor`.
ceedubs added a commit to ceedubs/cats that referenced this issue May 14, 2016
Resolves typelevel#888

I don't think there's really a _right_ answer here. Both methods of
combining `Xor`s are straightforward and law-abiding. However, I think
that since in pretty much every other context (including the
`SemigroupK` instances), `Xor` does not combine failures and `Validated`
does, it is less surprising behavior to not combine left values in
`combine` methods for `Xor` and `XorT`.

Some notes about related implementations:
- Scalaz's respective methods (and semigroup instances) _do_ combine
  errors, so this is a deviation from that.
- The `Semigroup` for `Either` in Haskell has doesn't combine values at
  all, but returns the first `Right` (if any), making it equivalent to
  the behavior of `orElse` and the `SemigroupK` instance for `Xor`.
  Since we have already decided to not go with `orElse`-like behavior
  for our `Option` semigroup, I'm inclined to not take this approach for
  `Xor`.

See also
typelevel#996 (comment)
@ceedubs ceedubs self-assigned this May 14, 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

No branches or pull requests

3 participants