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

Don't combine lefts on Xor and XorT combine #1034

Merged
merged 2 commits into from
May 19, 2016

Conversation

ceedubs
Copy link
Contributor

@ceedubs ceedubs commented May 14, 2016

Resolves #888

I don't think there's really a right answer here. Both methods of
combining Xors 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
#996 (comment)

cc @notxcain @aaronlevin @travisbrown @tpolecat @stew whom I think have all weighed in on this or similar items in the past

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)
case Xor.Right(b1) => that match {
case Xor.Left(a2) => Xor.Left(a2)
case l @ Xor.Left(_) => l
Copy link
Contributor

Choose a reason for hiding this comment

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

Tiny thing, but let's call this left instead of l -- I think it reads a lot better.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍 updated

@non
Copy link
Contributor

non commented May 16, 2016

Tiny style comment, but 👍 from me!

@aaronlevin
Copy link
Contributor

aaronlevin commented May 16, 2016

👍 . This makes combine semantically equivalent to:

for {
   x <- this
   y <- that
} yield SemigroupInstance.combine(x,y)

which make sense as you mostly want to use Xor for its monadic behaviour.

@codecov-io
Copy link

Current coverage is 88.84%

Merging #1034 into master will increase coverage by <.01%

  1. 2 files (not in diff) in ...n/scala/cats/functor were modified. more
  2. 2 files (not in diff) in .../src/main/scala/cats were modified. more
    • Hits +1
  3. File .../cats/data/Xor.scala was modified. more
    • Misses -1
    • Partials 0
    • Hits +1
@@             master      #1034   diff @@
==========================================
  Files           215        215          
  Lines          2726       2724     -2   
  Methods        2664       2663     -1   
  Messages          0          0          
  Branches         57         57          
==========================================
  Hits           2420       2420          
+ Misses          306        304     -2   
  Partials          0          0          

Sunburst

Powered by Codecov. Last updated by 3febb06...faf05f9

@non
Copy link
Contributor

non commented May 17, 2016

👍

@ceedubs ceedubs merged commit c134847 into typelevel:master May 19, 2016
@ceedubs ceedubs deleted the xor-combine branch May 19, 2016 13:34
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.

4 participants