Add SemigroupK instance for Xor + tests #996
Conversation
Xor's SemigroupK instance follows that of XorT.
|
Note: one difference from the |
|
Build failure ended with: |
|
There is another possible implementation of Thoughts? I'd be happy to update and change if people feel either way. I like this because it's more semantically closer to /cc @ceedubs |
|
@aaronlevin thanks! We are currently having an issue with builds failing that we haven't tracked down, so I don't think the build failure has anything to do with your changes. Hopefully we get that figured out shortly.
I would have done the same as you. In some cases defining a separate trait can be handy for code reuse (for example if you have a
I have the same inclination. It's a little troubling that this would be inconsistent with the Sorry that a seemingly simple change turned into a complicated discussion! |
|
It occurs to me that the reason for the approach taken in scalaz's |
|
Closing and reopening to pick up build fixes. |
Current coverage is 83.76%@@ master #996 diff @@
========================================
Files 184 368 +184
Lines 2183 2364 +181
Methods 0 2160 +2160
Branches 43 15 -28
========================================
- Hits 1983 1980 -3
- Misses 200 384 +184
Partials 0 0
|
|
@ceedubs I'm inclined to agree. Taking the semantic, right-biased interpretation of I'll proceed with the changes in a bit and if we have to roll-back, that's fine. This will be a breaking change, unfortunately, as it may require putting constraints on the |
|
@aaronlevin thanks for being so willing to make changes.
I thought we were on the same page about simply removing the We aren't too concerned about breaking changes until the upcoming 1.0 release, so it's best to fix it now even if it is a breaking change. |
|
@ceedubs ah, sorry, I can remove the |
We remove the MonoidK instance as it depended on an not-well defined SemigroupK instance. The new SemigroupK instance places no constraints on `L`, and has a "first Right or Last Left" behaviour for combine. This has a closer semantic meaning to Xor(T)'s "success or failure" interpretation. It didn't make sense previously that the empty value for XortT's MonoidK instance represented failure (`XortT.left(F.pure(L.empty))(F)`). Why should every empty value signal failure?
Update Xor's SemigroupK instance to be aligned with XorT's, with first-right, last-left semnatics.
|
@ceedubs updated! :) |
| @@ -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] | |||
There was a problem hiding this comment.
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 :)
There was a problem hiding this comment.
ah, good call. this is what I get for submitting a PR at 8am before breakfast :)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
😈
|
@aaronlevin I suggested a couple very minor (and admittedly nitpicky) changes to the tests, but this looks good to me. Are other people okay with removing the |
|
@ceedubs feedback has been incorporated (thanks!) |
|
👍 fantastic. Thanks a bunch @aaronlevin! |
|
@ceedubs \o/ |
|
closing and reopening to see if it makes the code coverage report make more sense |
|
Looks good to me. 👍 Thanks @aaronlevin! |
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)
Xor's SemigroupK instance follows that of XorT.