Add syntax to create a XorT from an F[A] and from a Xor. #1155

peterneyens · 2016-06-20T19:42:38Z

I have added syntax to create a XorT from a

F[A] : fa.leftXorT and fa.rightXorT and which use XorT.left and XorT.right
Xor : xor.toXorT (which was suggested by @ceedubs in Add convenience wrapper for Xor.fromOption in XorT #1078)

The last one is a little bit difficult:

If I define it on Xor I need to work around the covariance of A and B in Xor versus the invariance in XorT, so you would need to write xor.toXorT[F, A, B].
I have also defined toXorT (currently toXorT2 in syntax.xor) as a syntax function on Xor so you can write xor.toXorT[F]. But it's probably not expected that there are syntax functions for a data type defined in cats itself (at least I couldn't think of / find an earlier precedent).

I prefer the syntax function because it is nicer/easier to use, but it would probably be more difficult to find, since it won't be in the Xor scaladoc. WDYT ?

kailuowang · 2016-06-20T20:17:53Z

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

@@ -228,7 +229,7 @@ class XorTests extends CatsSuite {
    }
  }

-  test("combine is right iff both operands are right") {
+  test("combine is right if both operands are right") {
    forAll { (x: Int Xor String, y: Int Xor String) =>


hmmm, I wonder why you changed this. This test seems testing iff to me

Oops, thought it was a typo 😉

codecov-io · 2016-06-20T20:32:47Z

Current coverage is 88.78%

Merging #1155 into master will decrease coverage by 0.01%

@@             master      #1155   diff @@
==========================================
  Files           232        233     +1   
  Lines          3073       3077     +4   
  Methods        3021       3024     +3   
  Messages          0          0          
  Branches         49         50     +1   
==========================================
+ Hits           2729       2732     +3   
- Misses          344        345     +1   
  Partials          0          0

Powered by Codecov. Last updated by ed21654...258fac6

kailuowang · 2016-06-21T11:38:15Z

core/src/main/scala/cats/data/Xor.scala

+   * }}}
+   */
+  def toXorT[F[_], L, R](implicit F: Applicative[F], evL: A <:< L, evR: B <:< R): XorT[F, L, R] =
+    XorT(F.pure(bimap(evL, evR)))


Hmmm if toXorT has to respecify all type parameters I am not sure I will favor it over fromXor

Indeed, that is what I was thinking. What do you think about adding it as a syntax method (see syntax.xor.toXorT2) ?

Yeah, I wasn't thinking about the variance on Xor when I proposed this previously. The syntax approach doesn't look bad, but I want to ponder our options a bit.

kailuowang · 2016-06-21T11:50:47Z

I vote for the toXorT2 in syntax. The one on data class itself is hard to use and doesn't feel right to me. The users I know are used to find methods in syntax so discoverability doesn't seem an issue to me.

ceedubs · 2016-06-24T11:53:02Z

For anyone who has asked about what the downsides of variance are, this is an example :)

What if we were to put something like this in the Xor companion object?

  final implicit class XorOps[A, B](val value: A Xor B) extends AnyVal {
    def toXorT[F[_]:Applicative]: XorT[F, A, B] = XorT.fromXor(value)
  }

If we do that then people should automatically pick up the syntax on their Xor instances and therefore it will probably show up in their IDE's autocomplete. As you've mentioned, people probably won't expect to need an import to add this sort of method onto Xor, which is why I'm inclined to go down this route as opposed to the normal syntax route; I would view this not being directly on Xor as an implementation detail and not something that people should have to think about.

We could also link to XorOps within the Xor scaladoc with a note like "some additional Xor methods can be found here" and maybe a brief explanation of why they are separate.

What do you think?

ceedubs · 2016-06-27T11:13:42Z

core/src/main/scala/cats/syntax/xort.scala

+    new XorTOps[U.M, U.A](U.subst(fa))(U.TC)
+}
+
+final class XorTOps[F[_]: Functor, A](val fa: F[A]) {


Should we call this XorTFunctorOps or something? We could conceivably want to add XorT-related syntax onto types other than F[A]. I think it should still be mixed in via XorTSyntax though.

kailuowang · 2016-06-29T11:04:52Z

Many thanks! 👍

ceedubs · 2016-06-29T11:19:05Z

core/src/main/scala/cats/syntax/xort.scala

+  implicit def catsSyntaxXorT[F[_]: Functor, A](fa: F[A]): XorTFunctorOps[F, A] = new XorTFunctorOps(fa)
+}
+
+trait XorTSyntax1 {


We probably want to make this private (as per #1160).

peterneyens · 2016-06-30T13:31:40Z

Thanks for the comments @ceedubs and @kailuowang. Even a simple PR like this looks a lot nicer after improving the little issues you pointed out.

ceedubs · 2016-06-30T14:22:39Z

👍 excellent. Thank you @peterneyens!

peterneyens force-pushed the creating-xort-from-xor-or-fa branch from d0d8fe0 to 7a7eb70 Compare June 20, 2016 19:54

kailuowang reviewed Jun 20, 2016
View reviewed changes

peterneyens force-pushed the creating-xort-from-xor-or-fa branch from 7a7eb70 to d075224 Compare June 20, 2016 20:32

peterneyens force-pushed the creating-xort-from-xor-or-fa branch from d075224 to 462d3e8 Compare June 21, 2016 05:56

kailuowang reviewed Jun 21, 2016
View reviewed changes

peterneyens force-pushed the creating-xort-from-xor-or-fa branch 2 times, most recently from dc8f34a to 374fbeb Compare June 25, 2016 11:41

ceedubs reviewed Jun 27, 2016
View reviewed changes

peterneyens force-pushed the creating-xort-from-xor-or-fa branch from 374fbeb to 3948f34 Compare June 29, 2016 09:09

ceedubs reviewed Jun 29, 2016
View reviewed changes

Add syntax to create a XorT from an F[A] and from a Xor.

258fac6

peterneyens force-pushed the creating-xort-from-xor-or-fa branch from 3948f34 to 258fac6 Compare June 30, 2016 12:41

ceedubs merged commit 5100ab5 into typelevel:master Jun 30, 2016

peterneyens deleted the creating-xort-from-xor-or-fa branch July 18, 2016 12:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add syntax to create a XorT from an F[A] and from a Xor. #1155

Add syntax to create a XorT from an F[A] and from a Xor. #1155

peterneyens commented Jun 20, 2016 •

edited

Loading

kailuowang Jun 20, 2016

peterneyens Jun 20, 2016

codecov-io commented Jun 20, 2016 •

edited

Loading

kailuowang Jun 21, 2016

peterneyens Jun 21, 2016

ceedubs Jun 21, 2016

kailuowang commented Jun 21, 2016

ceedubs commented Jun 24, 2016

ceedubs Jun 27, 2016

kailuowang commented Jun 29, 2016

ceedubs Jun 29, 2016

peterneyens commented Jun 30, 2016

ceedubs commented Jun 30, 2016

Add syntax to create a XorT from an F[A] and from a Xor. #1155

Add syntax to create a XorT from an F[A] and from a Xor. #1155

Conversation

peterneyens commented Jun 20, 2016 • edited Loading

kailuowang Jun 20, 2016

Choose a reason for hiding this comment

peterneyens Jun 20, 2016

Choose a reason for hiding this comment

codecov-io commented Jun 20, 2016 • edited Loading

Current coverage is 88.78%

kailuowang Jun 21, 2016

Choose a reason for hiding this comment

peterneyens Jun 21, 2016

Choose a reason for hiding this comment

ceedubs Jun 21, 2016

Choose a reason for hiding this comment

kailuowang commented Jun 21, 2016

ceedubs commented Jun 24, 2016

ceedubs Jun 27, 2016

Choose a reason for hiding this comment

kailuowang commented Jun 29, 2016

ceedubs Jun 29, 2016

Choose a reason for hiding this comment

peterneyens commented Jun 30, 2016

ceedubs commented Jun 30, 2016

peterneyens commented Jun 20, 2016 •

edited

Loading

codecov-io commented Jun 20, 2016 •

edited

Loading