Add SemigroupK instance for Xor + tests #996

aaronlevin · 2016-04-24T17:03:26Z

Xor's SemigroupK instance follows that of XorT.

aaronlevin · 2016-04-24T17:04:16Z

Note: one difference from the SemigroupK instance for XorT is that XorT seemed to create a separate trait (XorTSemigroupK) and I wasn't sure why this was necessary so I didn't follow suit. I'm happy to update if there's a reason for this (am also generally curious)

aaronlevin · 2016-04-24T17:14:13Z

Build failure ended with: The command "scripts/travis-publish.sh" exited with 6. Unsure how to proceed.

aaronlevin · 2016-04-24T18:05:53Z

There is another possible implementation of SemigroupK[Xor[E,?]] where we take the first Right or the last Left and put no constraints on E. This would give a SemigroupK for any Xor. This is also identical to the Semigroup instance for Either in haskell.

Thoughts? I'd be happy to update and change if people feel either way.

I like this because it's more semantically closer to Xor, whereas if you want the Semigroup constraint on E then you can use Validated.

/cc @ceedubs

ceedubs · 2016-04-25T11:33:59Z

@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.

Note: one difference from the SemigroupK instance for XorT is that XorT seemed to create a separate trait (XorTSemigroupK) and I wasn't sure why this was necessary so I didn't follow suit. I'm happy to update if there's a reason for this (am also generally curious)

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 Semigroup instance as long as there's a Semigroup[E] but you also have a Monoid instance if there's a Monoid[E]). Some people prefer to always create a separate trait/class as opposed to creating a new anonymous type. I guess I'm not exactly sure what the motivation is there - perhaps they just like being a bit more explicit? I think @non might fall into that camp, so he might weigh in. Either way, I think what you have done is consistent with many parts of the cats codebase and isn't a problem.

I like this because it's more semantically closer to Xor, whereas if you want the Semigroup constraint on E then you can use Validated.

I have the same inclination. It's a little troubling that this would be inconsistent with the SemigroupK instance for XorT though. I'm guessing that the XorT instance followed a precedent set in scalaz without too much thought into whether we wanted to take the same approach in cats. I'm wondering if it would make sense to change it. @xuwei-k do you have any thoughts on this?

Sorry that a seemingly simple change turned into a complicated discussion!

aaronlevin · 2016-04-25T12:30:20Z

@ceedubs Good call on alignment with XorT. I suggest we:

wait to hear back from @xuwei-k on anything about XorT or thoughts.
I can incorporate any changes, including updating both Xor and XorT to have the desired "first-right, last-left" semantics.

ceedubs · 2016-04-25T12:47:19Z

It occurs to me that the reason for the approach taken in scalaz's EitherT (equivalent to cats XorT) is probably because it also provides a MonadPlus instance (equivalent to cats MonadCombine), which needs to be consistent with the Plus instance (equivalent to cats SemigroupK). The MonadCombine instance for XorT was removed in cats because it violates right absorbtion, right distributivity, and left distributivity. Cats does, however still provide a MonoidK instance for XorT, that its SemigroupK instance should be consistent with. Personally, I'm not a big fan of that MonoidK instance and wouldn't mind seeing it go away. It seems kind of arbitrary to me to claim that the empty XorT[F, L, A] is based on the empty of the monoid of the left type.

ceedubs · 2016-04-25T13:51:46Z

Closing and reopening to pick up build fixes.

codecov-io · 2016-04-25T15:28:42Z

Current coverage is 83.76%

Merging #996 into master will increase coverage by -7.07%

@@           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

2 files (not in diff) in ...main/scala/cats/laws were modified. more
2 files (not in diff) in .../main/scala/cats/std were modified. more
3 files (not in diff) in .../src/main/scala/cats were modified. more
2 files in .../src/main/scala/cats were modified. more
- Hits +1
38 files (not in diff) in ...cats/laws/discipline were created. more
36 files (not in diff) in ...main/scala/cats/laws were created. more
36 files (not in diff) in ...in/scala/cats/syntax were created. more
14 files (not in diff) in .../main/scala/cats/std were created. more
4 files (not in diff) in ...n/scala/cats/functor were created. more
6 files (not in diff) in ...main/scala/cats/free were created. more

Powered by Codecov. Last updated by cb49c71

aaronlevin · 2016-04-25T16:43:59Z

@ceedubs I'm inclined to agree. Taking the semantic, right-biased interpretation of Xor/XorT as success-or-failure, then it really doesn't make sense that an empty value should correspond to failure.

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 monoidK instance.

ceedubs · 2016-04-25T17:06:48Z

@aaronlevin thanks for being so willing to make changes.

This will be a breaking change, unfortunately, as it may require putting constraints on the monoidK instance.

I thought we were on the same page about simply removing the MonoidK instance for XorT, so this didn't quite make sense to me. Am I misunderstanding?

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.

aaronlevin · 2016-04-26T07:05:20Z

@ceedubs ah, sorry, I can remove the MonoidK instance :)

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.

aaronlevin · 2016-04-26T07:35:13Z

@ceedubs updated! :)

ceedubs · 2016-04-26T10:53:52Z

tests/src/test/scala/cats/tests/XorTests.scala

@@ -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]


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 :)

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

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.

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.

😈

ceedubs · 2016-04-26T11:00:21Z

@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 MonoidK instance for XorT?

aaronlevin · 2016-04-26T11:29:22Z

@ceedubs feedback has been incorporated (thanks!)

ceedubs · 2016-04-26T12:38:47Z

👍 fantastic. Thanks a bunch @aaronlevin!

aaronlevin · 2016-04-26T12:41:44Z

@ceedubs \o/

ceedubs · 2016-04-26T19:34:51Z

closing and reopening to see if it makes the code coverage report make more sense

non · 2016-04-27T12:39:55Z

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)

Add SemigroupK instance for Xor + tests

63d186b

Xor's SemigroupK instance follows that of XorT.

ceedubs closed this Apr 25, 2016

ceedubs reopened this Apr 25, 2016

ceedubs added the in progress label Apr 25, 2016

aaron levin added 2 commits April 26, 2016 09:25

Xor: SemigroupK w/ first-right,last-left semantics

0bae0ca

Update Xor's SemigroupK instance to be aligned with XorT's, with first-right, last-left semnatics.

ceedubs reviewed Apr 26, 2016
View reviewed changes

aaron levin added 2 commits April 26, 2016 13:24

Remove implct Semigroup from Xor{T} SemigroupK tests

8636343

Move Xor{T} SemigroupK tests into broader scope

db83772

ceedubs closed this Apr 26, 2016

ceedubs reopened this Apr 26, 2016

ceedubs removed the in progress label Apr 26, 2016

ceedubs added the in progress label Apr 26, 2016

ceedubs mentioned this pull request Apr 27, 2016

Confusion about Instances{peano}: document reasoning behind Instances0, Instances1, .. #997

Open

non merged commit ca8e4f5 into typelevel:master Apr 27, 2016

non removed the in progress label Apr 27, 2016

ceedubs mentioned this pull request May 14, 2016

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

Merged

adelbertc mentioned this pull request Aug 16, 2016

Replace Xor with Either, fixes #1192 #1289

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add SemigroupK instance for Xor + tests #996

Add SemigroupK instance for Xor + tests #996

aaronlevin commented Apr 24, 2016

aaronlevin commented Apr 24, 2016

aaronlevin commented Apr 24, 2016

aaronlevin commented Apr 24, 2016

ceedubs commented Apr 25, 2016

aaronlevin commented Apr 25, 2016

ceedubs commented Apr 25, 2016

ceedubs commented Apr 25, 2016

codecov-io commented Apr 25, 2016 •

edited

Loading

aaronlevin commented Apr 25, 2016

ceedubs commented Apr 25, 2016

aaronlevin commented Apr 26, 2016

aaronlevin commented Apr 26, 2016

ceedubs Apr 26, 2016

aaronlevin Apr 26, 2016

aaronlevin Apr 26, 2016

ceedubs Apr 26, 2016 •

edited

Loading

ceedubs commented Apr 26, 2016

aaronlevin commented Apr 26, 2016

ceedubs commented Apr 26, 2016

aaronlevin commented Apr 26, 2016

ceedubs commented Apr 26, 2016

non commented Apr 27, 2016

Add SemigroupK instance for Xor + tests #996

Add SemigroupK instance for Xor + tests #996

Conversation

aaronlevin commented Apr 24, 2016

aaronlevin commented Apr 24, 2016

aaronlevin commented Apr 24, 2016

aaronlevin commented Apr 24, 2016

ceedubs commented Apr 25, 2016

aaronlevin commented Apr 25, 2016

ceedubs commented Apr 25, 2016

ceedubs commented Apr 25, 2016

codecov-io commented Apr 25, 2016 • edited Loading

Current coverage is 83.76%

aaronlevin commented Apr 25, 2016

ceedubs commented Apr 25, 2016

aaronlevin commented Apr 26, 2016

aaronlevin commented Apr 26, 2016

ceedubs Apr 26, 2016

Choose a reason for hiding this comment

aaronlevin Apr 26, 2016

Choose a reason for hiding this comment

aaronlevin Apr 26, 2016

Choose a reason for hiding this comment

ceedubs Apr 26, 2016 • edited Loading

Choose a reason for hiding this comment

ceedubs commented Apr 26, 2016

aaronlevin commented Apr 26, 2016

ceedubs commented Apr 26, 2016

aaronlevin commented Apr 26, 2016

ceedubs commented Apr 26, 2016

non commented Apr 27, 2016

codecov-io commented Apr 25, 2016 •

edited

Loading

ceedubs Apr 26, 2016 •

edited

Loading