Add convenience wrapper for Xor.fromOption in XorT

[mkotsbak]

No description provided.

[non]

Seems like you are missing an implicit parameter here.

[mkotsbak]

Japp, pushed fix now.

[codecov-io]

Current coverage is 88.32%

Merging #1078 into master will increase coverage by 0.18%

1. 2 files (not in diff) in .../main/scala/cats/std were modified. more
   • Misses +3
2. 3 files (not in diff) in ...n/scala/cats/functor were modified. more
   • Hits -3
3. File ...a/cats/std/try.scala (not in diff) was deleted. more

@@             master      #1078   diff @@
==========================================
  Files           224        223     -1   
  Lines          2842       2809    -33   
  Methods        2785       2753    -32   
  Messages          0          0          
  Branches         52         51     -1   
==========================================
- Hits           2505       2481    -24   
+ Misses          337        328     -9   
  Partials          0          0          

Powered by Codecov. Last updated by b149835...9665410

[peterneyens]

With the current fromOption you need to specify all the type parameters, because you need F.

val foo: Option[Int] = ???
XorT.fromOption[Future, String, Int](foo, "Couldn't find foo")

Maybe you could create a FromOptionPartiallyApplied class similar to the classes used by fromXor and fromEither to make it possible to only specify F ?

The code above could then look like :

XorT.fromOption[Future](foo, "Couldn't find foo")

[mkotsbak]

Good idea, updated like that.

Btw, do you know if fromOptionT[F, _] also should be added, or is it supported somehow else?

[peterneyens]

OptionT already has the methods toLeft and toRight which return a XorT.

[mkotsbak]

Ajapp, those are probably best to use.

[ceedubs]

@mkotsbak I'm wondering if a more general solution might be to add a def liftXorT[F[_]:Applicative]: XorT[F, A, B] to Xor. That way you could use something like myOption.toRightXor(err).liftXorT[Future] for the use-case presented in this PR, but you also just have a general mechanism for lifting Xors into XorTs. What do you think?

[mkotsbak]

Well, yes, I suppose that is the same as fromXor just in the opposite direction, but possibly requiring specifying less types explicit, almost like typing this manually:

fromXor[F](Xor.fromOption(opt, ifNone))

Maybe we could have both available.

[peterneyens]

I can see the following ways how we can do this at the moment (without this PR):

XorT.fromXor[Future](1.some.toRightXor("kamboom"))
OptionT.fromOption[Future](1.some).toRight("kamboom")

I don't think the first one is much worse than XorT.fromOption[Future](1.some, "kamboom").

@ceedubs Your Xor.liftXorT would then be similar to XorT.fromXor ? I don't think we need both (even though calling xor.liftXorT[F] is probably nicer than XorT.fromXor[F](xor) ? Also when I see liftG I expect something like F[A] => G[F, A] while this is Xor[A, B] => XorT[F, A, B].

[ceedubs]

@peterneyens yeah, I guess you are right that my liftXorT would be pretty much the same as fromXor and that liftX tends to be what you said. Maybe toXorT would be a better name? I guess the only real differences are that it wouldn't need to go through a *PartiallyApplied method and if you are chaining methods sometimes calling .toXorT at the end is more convenient than wrapping the whole block in XorT.fromXor. Actually I'm now a bit inclined to add .toXorT and remove fromXor, but I also don't have a problem with both being there. @adelbertc it looks like you added fromXor -- what are your thoughts?

I tend to agree with @peterneyens that it's not that much more verbose to use the existing methods in cats. I'm hesitant to add this, because it seems like if we are adding FromOptionPartiallyApplied then we should also be adding FromValidatedPartiallyApplied, FromEitherPartiallyApplied, FromTryPartiallyApplied, etc, and it seems like it would be more straightforward to just make sure that it's easy to turn these types into an Xor and to convert that Xor into an XorT.

edit: oops it looks like FromEitherPartiallyApplied already exists, which I guess only goes to validate my concern :P

[peterneyens]

Xor.toXorT would indeed be a better name and I also think it would be used more often than XorT.fromXor. Similar for (a possible) Option.toOptionT and OptionT.fromOption.

It seems there already is an XorT.fromEither.

[mkotsbak]

Ok, so now with the other PR we have:
myOption.toRightXor(err).toXorT[Future]

with workaround:

XorT.fromXor[Future](myOption.toRightXor(err))

so then I think we don't need fromOption so I close this PR.

[mkotsbak]

Hmm, but there is still no myOption.toRightXor? Yes, it is if cats.implicits._ is imported.