Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add OptionT.liftF and update related docs and tests. Close issue #659 #667

Merged
merged 3 commits into from
Nov 17, 2015
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions AUTHORS.md
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ This file lists the people whose contributions have made Cats
possible:

* Adelbert Chang
* Alessandro Lacava
* Alissa Pajer
* Alistair Johnson
* Amir Mohammad Saied
Expand Down
6 changes: 6 additions & 0 deletions core/src/main/scala/cats/data/OptionT.scala
Original file line number Diff line number Diff line change
Expand Up @@ -120,6 +120,12 @@ object OptionT extends OptionTInstances {
def apply[A](value: Option[A])(implicit F: Applicative[F]): OptionT[F, A] =
OptionT(F.pure(value))
}

/**
* Lifts the `F[A]` Functor into an `OptionT[F, A]`.
*
*/
def liftF[F[_], A](fa: F[A])(implicit F: Functor[F]): OptionT[F, A] = OptionT(F.map(fa)(Option(_)))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This may not really be something to worry about, but I'm thinking that maybe this should use Some(_) instead of Option(_). The Option.apply method has logic to turn null into None. In Cats we are generally assuming that people aren't passing around null. But if for some reason they really want to lift a null value into the OptionT structure, I don't think we should prevent them from doing that here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I used Option(_) to prevent null and getting None instead of Some(null) in that case, but if you confirm we should not prevent them from doing that I'll change it to Some(_).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that it should be consistent with our Applicative[Option].pure implementation, which uses Some: /~https://github.com/non/cats/blob/master/core/src/main/scala/cats/std/option.scala#L14. So yeah, I think we should change this here. I don't think we should conflate the issues of lifting and de-nulling.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I totally agree! I'll change it to Some(_). Thank you.

}

private[data] sealed trait OptionTInstances1 {
Expand Down
27 changes: 27 additions & 0 deletions docs/src/main/tut/optiont.md
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,33 @@ val noWelcome: OptionT[Future, String] = customGreetingT.filterNot(_.contains("w
val withFallback: Future[String] = customGreetingT.getOrElse("hello, there!")
```

## From `Option[A]` and/or `F[A]` to `OptionT[F, A]`

Sometimes you may have an `Option[A]` and/or `F[A]` and want to *lift* them into an `OptionT[F, A]`. For this purpose `OptionT` exposes two useful methods, namely `fromOption` and `liftF`, respectively. E.g.:

```tut:silent
import scala.concurrent.Future
import scala.concurrent.ExecutionContext.Implicits.global

import cats.data.OptionT
import cats.std.future._
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These imports already happen earlier in the file. Do we want them again here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I noticed but I wanted to example to be self-contained.

By the way, if you don't import scala.concurrent.ExecutionContext.Implicits.global you get a misleading:

scala> OptionT.liftF(f)
<console>:17: error: could not find implicit value for parameter F: cats.Functor[scala.concurrent.Future]
       OptionT.liftF(f)

Do you think I should file this in a different issue or it's not a problem? I mean, even if you import cats.std.future._ you get that error if no ExecutionContext is found in scope and people could get confused by this because they would think the problem is it cannot find a Functor[Future] instance while it actually can't just find the ExecutionContext.

Anyway if you think it's not a problem I can remove those imports.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can see the benefit in self-contained examples. My only concern is that currently with tut I don't think we have a way to clear previous imports in the same file. So we run the risk of duplicating imports without actually guaranteeing that the example is self-contained. I don't have any really strong feelings on this though, so whatever you think is best.

Regarding the misleading missing implicit value message:
This came up in the gitter channel yesterday. I don't particularly like any of the solutions that we have in mind, but if there's something we can do to help, then we should. If you want to file an issue for that, that would be great!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I removed those imports and filed the #670 for that issue.


val greetingFO: Future[Option[String]] = Future.successful(Some("Hello"))

val firstnameF: Future[String] = Future.successful("Jane")

val lastnameO: Option[String] = Some("Doe")

val ot: OptionT[Future, String] = for {
g <- OptionT(greetingFO)
f <- OptionT.liftF(firstnameF)
l <- OptionT.fromOption(lastnameO)
} yield s"$g $f $l"

val result: Future[Option[String]] = ot.value // Future(Some("Hello Jane Doe"))

```

## Beyond map

Sometimes the operation you want to perform on an `Option[Future[String]]` might not be as simple as just wrapping the `Option` method in a `Future.map` call. For example, what if we want to greet the customer with their custom greeting if it exists but otherwise fall back to a default `Future[String]` greeting? Without `OptionT`, this implementation might look like:
Expand Down
6 changes: 6 additions & 0 deletions tests/src/test/scala/cats/tests/OptionTTests.scala
Original file line number Diff line number Diff line change
Expand Up @@ -111,6 +111,12 @@ class OptionTTests extends CatsSuite {
}
}

test("liftF") {
forAll { (xs: List[Int]) =>
xs.map(Option(_)) should ===(OptionT.liftF(xs).value)
}
}

test("show"){
val xor: String Xor Option[Int] = Xor.right(Some(1))
OptionT[Xor[String, ?], Int](xor).show should === ("Xor.Right(Some(1))")
Expand Down