Add toNested to OptionT and XorT.

[peterneyens]

I have added toNested to OptionT and XorT like I mentioned in #1107.

This among other things makes it possible to use the "applicative ap" (called app in Scalaz) which is inconsistent the ap it gets from the monad instances of OptionT or XorT (see #1106).

import cats.implicits._
import cats.data.OptionT

val ff: OptionT[List, Int => String] = 
  OptionT(List(Option(_.toString), none[Int => String]))
val fa: OptionT[List, Int] = 
  OptionT(List(1.some, 2.some))

ff.ap(fa)
// OptionT[List,String] = OptionT(List(Some(1), Some(2), None))

OptionT(ff.toNested.ap(fa.toNested).value)
// OptionT[List,String] = OptionT(List(Some(1), Some(2), None, None))

The toNested for XorT is currently not that useful because we are not able to get the implicits for Nested[F, A Xor ?, B], but it works with the SI-2712 fix :

import cats.data.{Xor, XorT}
import cats.implicits._

val ff2: XorT[List, String, Int => String] = 
  XorT(List(Xor.right(_.toString), "error".left))
val fa2: XorT[List, String, Int] = 
  XorT(List(1.right, 2.right))

ff2.ap(fa2)
// XorT[List,String,String] = XorT(List(Right(1), Right(2), Left(error)))

XorT(ff2.toNested.ap(fa2.toNested).value)
// XorT[List,String,String] = XorT(List(Right(1), Right(2), Left(error), Left(error)))

---

In #1107 I also suggested that we could simplify some functions like foldLeft by using the one from Nested, but I benchmarked optiont.foldLeft(0)(_ + _) (the current implementation) vs optiont.toNested.foldLeft(0)(_ + _) and the one using toNested is quite a bit slower :

[info] Benchmark                    Mode  Cnt         Score         Error  Units
[info] ToNestedBench.compose       thrpt   10  58633849.651 ±  646010.783  ops/s
[info] ToNestedBench.nestedDetour  thrpt   10  39380592.834 ± 4962640.124  ops/s

Even though a foldLeft implementation with toNested would like slightly nicer, I am not sure it's worth the performance hit ? (This doesn't feel that important, but if someone wants to voice their opinion ...)

F.compose(optionInstance).foldLeft(value, b)(f)
toNested.foldLeft(value, b)(f)

---

I am not sure which tests to add (I could check for consistency for map, traverse, ... ?)

[ceedubs]

Nice @peterneyens. And thank you for the detailed explanation of the inconsistent ap and setting up a benchmark!

I am not sure which tests to add (I could check for consistency for map, traverse, ... ?)

Since this is such a trivial method, I'm not really concerned about testing it. Maybe you could add a simple doctest example so the code coverage doesn't drop? Actually the examples you gave in this PR along with comments about potential ap inconsistencies would probably be great for the scaladoc.

[codecov-io]

Current coverage is 88.80%

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

@@             master      #1172   diff @@
==========================================
  Files           233        233          
  Lines          3079       3081     +2   
  Methods        3025       3029     +4   
  Messages          0          0          
  Branches         51         49     -2   
==========================================
+ Hits           2734       2736     +2   
  Misses          345        345          
  Partials          0          0          

Powered by Codecov. Last updated by 3538140...80be15f

[peterneyens]

I have added the ap examples as ScalaDoc, the XorT one doesn't looks as nice because I had to work around SI-2712.

I am not sure which part of the diff codecov is complaining about ?

[ceedubs]

👍 looks great to me. Thanks for the detailed examples @peterneyens.

I am not sure which part of the diff codecov is complaining about ?

The "patch" code coverage stats often seem kind of bogus to me, though I'm sure this is just me not understanding what they represent. The overall project code coverage has gone up with this PR, so that's enough to make me happy.

[ceedubs]

@adelbertc as our resident Nested expert, would you be willing to review this PR?

[adelbertc]

LGTM 👍 Some questions:

1. Would we want to do this for other transformers like StateT, IdT, WriterT?
2. Should we add a withNested method similar to the withValidated method: https://github.com/typelevel/cats/blob/master/core/src/main/scala/cats/data/Xor.scala#L83 ?

[peterneyens]

1. I think OptionT and XorT are probably the most useful.

   We could define toNested on WriterT[F, L, V] to give us Nested[F, (L, ?), V], but we currently don't have instances for Tuple2[A, ?]. (I don' think Nested[F, Writer[L, ?], V] would make much sense ?)

   I don't know what StateT[F, S, A].toNested should give us ?

   IdT[F, A] to Nested[F, Id, A] would be easy, but I don't know how this could be useful (Though I have to admit that I have also never used IdT).

   Kleisli[F, A, B].toNested would be Nested[A => ?, F, B].

2. I think withNested could be useful. The ap example above could then be ff.withNested(_.ap(fa.toNested)).

I am not sure if these toNested methods would be used a lot ?

[ceedubs]

What's here looks really good to me and has a couple 👍 already, so I'm going to go ahead and merge it.

It sounds like there are some potential followups that could be addressed in later PRs. I agree with @peterneyens that toNested for the other transformers listed seems to be less useful. I could see withNested also being handy if you hit its use-case, but I'm not sure how often people will hit that -- either way is fine with me.