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.

Sign up for GitHub

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

Jump to bottom

Add when and unless to OptionT #3233

Merged
merged 2 commits into from
Jan 2, 2020
Merged

Add when and unless to OptionT #3233

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
82e8c7f
Add when and unless to OptionT
Dec 31, 2019
49be177
Fix comments
Jan 2, 2020
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
40 changes: 40 additions & 0 deletions core/src/main/scala/cats/data/OptionT.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -223,6 +223,46 @@ object OptionT extends OptionTInstances {
*/
def liftK[F[_]](implicit F: Functor[F]): F ~> OptionT[F, *] =
λ[F ~> OptionT[F, *]](OptionT.liftF(_))

/**
* Creates a non-empty `OptionT[F, A]` from an `A` value if the given condition is `true`.
* Otherwise, `none[F, A]` is returned. Analogous to `Option.when`.
*/
def when[F[_], A](cond: Boolean)(a: => A)(implicit F: Applicative[F]): OptionT[F, A] =
if (cond) OptionT.some[F](a) else OptionT.none[F, A]

/**
* Creates a non-empty `OptionT[F, A]` from an `F[A]` value if the given condition is `true`.
* Otherwise, the empty `OptionT[F, A]` is returned. Analogous to `Option.when`.
*/
def whenF[F[_], A](cond: Boolean)(fa: => F[A])(implicit F: Applicative[F]): OptionT[F, A] =
if (cond) OptionT.liftF(fa) else OptionT(F.map(fa)(_ => Option.empty))
Copy link
Member

Choose a reason for hiding this comment

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

I find the implementation of the else branch surprising. I would have thought that it is just OptionT.none[F, A] if cond is false (analogous to OptionT.when) but the current implementation runs the F-effect and then replaces the A with None: Option[A].

Copy link
Contributor Author

@strong-zero strong-zero Jan 2, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

It seems odd and it works when F = List and fa = List.empty because we want to get OptionT(List.empty) not OptionT(List(None)). But it breaks the case OptionT.whenF[List, Int](false)(List(1,2)) because we have OptionT(List(None, None)) not OptionT(List.empty)😱

I will create a follow-up pull request to fix it. I find no easy "fix" 😱
whenF uses liftF if cond is true. liftF is OptionT(F.map(fa)(Some(_))) so if cond is false, I think it should use something similar to liftF so that some "good" properties hold:

List(1, 2, 3).map(Option.when(false)(_)) == OptionT.whenF(false)(List(1, 2, 3)).value

List(1, 2, 3).map(Option.when(true)(_)) == OptionT.whenF(true)(List(1, 2, 3)).value

List().map(Option.when(true)(_)) == OptionT.whenF(true)(List()).value

List().map(Option.when(false)(_)) == OptionT.whenF(false)(List()).value

I should have implemented the case when F assumes the empty value or F is a Monoid separately. Any comments and suggestions are welcome!

Copy link
Member

Choose a reason for hiding this comment

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

If these properties imply that val launchMissiles: IO[Unit] = ...; OptionT.whenF(cond)(launchMissiles) always launches the missiles, no matter what cond is, then I think we should get rid of either these properties or whenF. I would prefer the former because whenF seems like a useful utility to me.

Copy link
Member

Choose a reason for hiding this comment

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

I agree with @fthomas. Good catch.

How about this for whenF? Many of the F methods on transformers are specified in terms of Id, and passes with Frank's proposed change:

  test("OptionT.whenF[Id, A] consistent with Option.when") {
    def when[A] = (c: Boolean, a: A) => if (c) Some(a) else None
    forAll { (i: Int, b: Boolean) =>
      OptionT.whenF[Id, Int](b)(i).value should ===(when(b, i))
    }
  }

We might even relate it to .sequence for less trivial effects:

forAll { (i: List[Int], b: Boolean) =>
  OptionT.whenF[List, Int](b)(i).value should ===(when(b, i).sequence)
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@fthomas @rossabaker Thank you both for your helpful examples. I will make a follow-up pull request.


/**
* Same as `whenF`, but expressed as a FunctionK for use with mapK.
*/
def whenK[F[_]](cond: Boolean)(implicit F: Applicative[F]): F ~> OptionT[F, *] =
λ[F ~> OptionT[F, *]](OptionT.whenF(cond)(_))

/**
* Creates a non-empty `OptionT[F, A]` from an `A` if the given condition is `false`.
* Otherwise, `none[F, A]` is returned. Analogous to `Option.unless`.
*/
def unless[F[_], A](cond: Boolean)(a: => A)(implicit F: Applicative[F]): OptionT[F, A] =
OptionT.when(!cond)(a)

/**
* Creates an non-empty `OptionT[F, A]` from an `F[A]` if the given condition is `false`.
* Otherwise, the empty `OptionT[F, A]` is returned. Analogous to `Option.unless`.
*/
def unlessF[F[_], A](cond: Boolean)(fa: => F[A])(implicit F: Applicative[F]): OptionT[F, A] =
OptionT.whenF(!cond)(fa)

/**
* Same as `unlessF`, but expressed as a FunctionK for use with mapK.
*/
def unlessK[F[_]](cond: Boolean)(implicit F: Applicative[F]): F ~> OptionT[F, *] =
λ[F ~> OptionT[F, *]](OptionT.unlessF(cond)(_))
}

sealed abstract private[data] class OptionTInstances extends OptionTInstances0 {
Expand Down
42 changes: 41 additions & 1 deletion tests/src/test/scala/cats/tests/OptionTSuite.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
package cats
package tests

import cats.data.{Const, OptionT}
import cats.data.{Const, IdT, OptionT}
import cats.kernel.{Monoid, Semigroup}
import cats.kernel.laws.discipline.{EqTests, MonoidTests, OrderTests, PartialOrderTests, SemigroupTests}
import cats.laws.discipline._
Expand Down Expand Up @@ -278,6 +278,46 @@ class OptionTSuite extends CatsSuite {
}
}

test("OptionT.when[Id, A] consistent with the same implementation of Option.when") {
val when = (c: Boolean, j: Int) => if (c) Some(j) else None
forAll { (i: Int, b: Boolean) =>
OptionT.when[Id, Int](b)(i).value should ===(when(b, i))
}
}

test("OptionT.whenF[F, A] consistent with the same implementation of Option.when") {
val when = (c: Boolean, j: Int) => if (c) Some(j) else None
forAll { (li: List[Int], b: Boolean) =>
OptionT.whenF(b)(li).value should ===(li.map(when(b, _)))
}
}

test("OptionT.whenK and OptionT.whenF consistent") {
forAll { (li: List[Int], b: Boolean) =>
IdT(li).mapK(OptionT.whenK(b)).value should ===(OptionT.whenF(b)(li))
}
}

test("OptionT.unless[Id, A] consistent with the same implementation of Option.unless") {
val unless = (c: Boolean, j: Int) => if (!c) Some(j) else None
forAll { (i: Int, b: Boolean) =>
OptionT.unless[Id, Int](b)(i).value should ===(unless(b, i))
}
}

test("OptionT.unlessF[F, A] consistent with the same implementation of Option.unless") {
val unless = (c: Boolean, j: Int) => if (!c) Some(j) else None
forAll { (li: List[Int], b: Boolean) =>
OptionT.unlessF(b)(li).value should ===(li.map(unless(b, _)))
}
}

test("OptionT.unlessK and OptionT.unlessF consistent") {
forAll { (li: List[Int], b: Boolean) =>
IdT(li).mapK(OptionT.unlessK(b)).value should ===(OptionT.unlessF(b)(li))
}
}

test("orElse and orElseF consistent") {
forAll { (o1: OptionT[List, Int], o2: OptionT[List, Int]) =>
o1.orElse(o2) should ===(o1.orElseF(o2.value))
Expand Down