Add when and unless to OptionT#3233
Conversation
Codecov Report
@@ Coverage Diff @@
## master #3233 +/- ##
==========================================
+ Coverage 93.05% 93.05% +<.01%
==========================================
Files 376 376
Lines 7412 7413 +1
Branches 192 200 +8
==========================================
+ Hits 6897 6898 +1
Misses 515 515
Continue to review full report at Codecov.
|
rossabaker
left a comment
rossabaker
left a comment
There was a problem hiding this comment.
I like it. Scaladoc nit: we've used both, but more commonly "non-empty" than "nonempty".
rossabaker
left a comment
rossabaker
left a comment
There was a problem hiding this comment.
👍 Implementing laziness with call-by-name is something we've debated in Cats, but this seems consistent with both Option (as the base monad) and EitherT (as how Cats does monad transformers).
| * 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)) |
There was a problem hiding this comment.
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].
There was a problem hiding this comment.
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!
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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)
}There was a problem hiding this comment.
@fthomas @rossabaker Thank you both for your helpful examples. I will make a follow-up pull request.
6 participants
In scala 2.13, we have Option.{when, unless}. So should cats, I guess 🤔