Added orElse combinator to IO #2940
Conversation
|
This is what the |
| def orElse[B >: A](other: IO[B]): IO[B] = | ||
| handleErrorWith(_ => other) |
There was a problem hiding this comment.
If it just calls to handleErrorWith then (perhaps) it would be better to add such a method directly to Cats' ApplicativeError type class instead of just IO. Because apparently it would make orElse functionality uniformly available for all ApplicativeError implementers (which includes IO from Cats Effect 2.x, Monix Task, fs2.Stream, etc). Wdyt?
There was a problem hiding this comment.
It's already syntax on ApplicativeError. I'm not sure why it shouldn't also be on the type class.
Regardless, I think the trend on IO has been toward a rich interface without typeclass syntax. Adding it here is consistent with Either, EitherT, etc.
There was a problem hiding this comment.
I'm not sure why it shouldn't also be on the type class.
I'm not sure either. I wonder if there is any convention on that in general. The syntax was introduced in typelevel/cats#2243 but seems the option for adding it to the typeclass had not been discussed nor challenged.
In general, having a method on typeclass makes it possible to override it in a particular implementation (e.g. for the sake of optimization). Not sure if it may make sense to do that for this orElse though.
There was a problem hiding this comment.
It's already syntax on ApplicativeError.
Btw, the syntax for ApplicativeError is defined in this way:
def orElse(other: => F[A])(implicit F: ApplicativeError[F, E]): F[A] =
F.handleErrorWith(fa)(_ => other)Notice it takes other as a by-name param while the suggested addition to IO takes it as just a value.
Wouldn't it be better to keep these two methods in correspondence to each other? I.e. to declare the latter's other as a by-name parameter too? @rossabaker @djspiewak , wdyt?
There was a problem hiding this comment.
Right, good catch. This should definitely be a by-name.
There was a problem hiding this comment.
Btw, it's really OOPy but if we had inheritable syntax traits for the monad classes themselves, couldn't we get all this syntax for free and avoid oopsies? Similar to how I've suggested that the IO companion object itself should implement Async[IO].
There was a problem hiding this comment.
So the by-name parameter point is a really important one. If this already exists as syntax on ApplicativeError, then we need to be consistent with that.
There was a problem hiding this comment.
This is great! Can you please run sbt prePR though? Additionally, as @rossabaker pointed out, these are the same semantics as MonoidK[IO], so it might be worth adjusting that instance to delegate to orElse to avoid duplication and improve test coverage.
| @@ -458,7 +458,7 @@ sealed abstract class IO[+A] private () extends IOPlatform[A] { | |||
| * @return | |||
| */ | |||
| def orElse[B >: A](other: IO[B]): IO[B] = | |||
| handleErrorWith(_ => other) | |||
| SemigroupK[IO].combineK(this, other) | |||
There was a problem hiding this comment.
I would go the other way around: leave this like you had it, and implement SemigroupK in terms of it.
There was a problem hiding this comment.
Thanks for the input. I would follow that approach.
| def orElse[B >: A](other: IO[B]): IO[B] = | ||
| handleErrorWith(_ => other) |
There was a problem hiding this comment.
So the by-name parameter point is a really important one. If this already exists as syntax on ApplicativeError, then we need to be consistent with that.
|
CI failure is because series/3.x is behind series/3.3.x so MiMa is rightfully upset :) merging #2947 should fix this, I'll look into that now. |
|
Closing / re-opening to trigger CI on a fresh merge commit, apologies. |
|
Congrats on your first PR! @walesho 🎉 |
Added orElse combinator to IO.
Example of Usage
val firstIO = IO.raiseError[Int]val secondIO = IO.pure(42)val program = firstIO orElse secondIO // this would result to secondIO