Add two new combinators: `cedeMap` and `intercede` #3353

biuld · 2023-01-09T16:41:05Z

This pull request resolves #3318.

armanbilge · 2023-01-09T17:44:31Z

core/shared/src/main/scala/cats/effect/IO.scala

+  /**
+   * Functor map, but causes a reschedule before and after `f`
+   */
+  def cedeMap[A, B](fa: IO[A])(f: A => B): IO[B] =
+    (fa <* cede).map(a => f(a)).guarantee(cede)
+
+  /**
+   * causes a reschedule before and after `fa`
+   */
+  def intercede[A](fa: IO[A]): IO[A] =
+    cede *> fa.guarantee(cede)


Thanks for opening the PR! What would you think about moving these methods onto the IO instance, so that instead of

IO.cedeMap(ioa)(a => foo(a)) IO.intercede(ioa)

users can write

ioa.cedeMap(a => foo(a)) ioa.intercede

However, the methods as written here would be an excellent addition to GenSpawn in the kernel module.

Let me know if you have any questions!

True! cedeMap and intercede as instance methods would be less boilerplate.

armanbilge · 2023-01-10T16:44:46Z

core/shared/src/main/scala/cats/effect/IO.scala

+  /**
+   * Functor map, but causes a reschedule before and after `f`. For more information, checkout
+   * `cede` in the companion object.
+   */
+  def cedeMap[B](f: A => B): IO[B] =
+    (this <* IO.cede).map(a => f(a)).guarantee(IO.cede)
+
+  /**
+   * Causes a reschedule before and after `fa`. For more information, checkout `cede` in the
+   * companion object.
+   */
+  def intercede: IO[A] =
+    IO.cede *> this.guarantee(IO.cede)


Nice!

Would you mind adding versions of these methods to GenSpawn in the kernel module? These would be more similar to your original definitions, which are in F[_] and take the effect as an argument (instead of instance methods).

cats-effect/kernel/shared/src/main/scala/cats/effect/kernel/GenSpawn.scala

Line 283 in 130fdcc

def cede: F[Unit]

armanbilge · 2023-01-11T14:38:15Z

kernel/shared/src/main/scala/cats/effect/kernel/GenSpawn.scala

+  def cedeMap[A, B](fa: F[A])(f: A => B): F[B] =
+    for {
+      a <- fa
+      _ <- cede
+      b = f(a)
+      _ <- cede
+    } yield b


Thanks for adding these! Is there a particular reason you wrote them as for ... yield? Unfortunately it's not quite correct, because it's missing the guarantee(...). Also, in my personal opinion, it's a bit harder to read :)

Is there a particular reason you wrote them as for ... yield?

No, it's I just can't find *> and <* combinators here. 🤦

Huh, were they not compiling? They come from here:

cats-effect/kernel/shared/src/main/scala/cats/effect/kernel/GenSpawn.scala

Line 22 in d6f9f68

import cats.syntax.all._

Oh, I missed the fact that MonadCancel is cats Monad! Will change that in a minute.

armanbilge · 2023-01-11T16:33:12Z

kernel/shared/src/main/scala/cats/effect/kernel/GenSpawn.scala

+  /**
+   * Functor map, but causes a reschedule before and after `f`
+   */
+  def cedeMap[A, B](fa: F[A])(f: A => B): F[B] =
+    (fa <* cede).map(a => f(a)).guarantee(cede)
+
+  /**
+   * Causes a reschedule before and after `fa`
+   */
+  def intercede[A](fa: F[A]): F[A] =
+    cede *> fa.guarantee(cede)


Excellent! :)

If you don't mind, there is one more place we need to add these methods, which is as syntax here:

cats-effect/kernel/shared/src/main/scala/cats/effect/kernel/syntax/GenSpawnSyntax.scala

Line 45 in 130fdcc

final class GenSpawnOps[F[_], A, E] private[syntax] (private val wrapped: F[A]) extends AnyVal {

These should delegate directly to the typeclass instance, instead of duplicating the implementation.

No problem!

armanbilge

Looking great! Just a couple docs tweaks.

armanbilge · 2023-01-12T22:14:25Z

kernel/shared/src/main/scala/cats/effect/kernel/GenSpawn.scala

+  /**
+   * Functor map, but causes a reschedule before and after `f`
+   */


Suggested change

/**

* Functor map, but causes a reschedule before and after `f`

*/

/**

* Like functor [[map]], but inserts a [[cede]] before and after `f`. Use this when `f` is a long-running, compute-bound operation.

*

* @see [[cede]] for more details

*/

armanbilge · 2023-01-12T22:17:16Z

kernel/shared/src/main/scala/cats/effect/kernel/GenSpawn.scala

+  /**
+   * Causes a reschedule before and after `fa`
+   */


Suggested change

/**

* Causes a reschedule before and after `fa`

*/

/**

* Insert a [[cede]] before and after `fa`. Use this when `fa` involves a long-running, compute-bound operation outside of the monadic context.

*

* @see [[cede]] for more details

*/

armanbilge · 2023-01-12T22:17:53Z

kernel/shared/src/main/scala/cats/effect/kernel/GenSpawn.scala

   */
  def cede: F[Unit]


Suggested change

*/

def cede: F[Unit]

*

* @see [[cedeMap]], [[intercede]]

*/

def cede: F[Unit]

armanbilge · 2023-01-12T22:18:24Z

kernel/shared/src/test/scala/cats/effect/kernel/SyntaxSpec.scala

+    {
+      val result = target.intercede
+      result: F[A]
+    }


Excellent, thanks for adding this test! Would you mind adding a test for cedeMap as well?

Actually the reason that I leave out the test for cedeMap is I don't know how to obtain f: A => B in test. I might need some time on getting familiar with how specs2 work. 😂

@biuld just add f: A => B to the function arguments here:

cats-effect/kernel/shared/src/test/scala/cats/effect/kernel/SyntaxSpec.scala

Line 107 in ed15013

def genSpawnSyntax[F[_], A, B, E](target: F[A], another: F[B])(implicit F: GenSpawn[F, E]) = {

armanbilge

👍 thanks for all your good work on this! But someone else should bikeshed the names because I just made them up :)

djspiewak · 2023-02-04T21:17:05Z

So I genuinely love the idea of these combinators. I'm not 100% sold on the API, but I'm thinking about it.

djspiewak · 2023-02-14T00:21:55Z

Going to punt these from 3.5 just to give us a bit more time to bikeshed. Like I said, I absolutely love the concept I just want to make sure we get the API right (not just the names, but also the ergonomics). Outstanding work, again!

BalmungSan

Leaving my two cents:

Personally I would prefer a more intuitive name like computeMap over cedeMap. Of course, the Scaladoc should mention and link cede.
In a similar vein, I would like to see two other static combinators like:

object IO {
  def compute[A](thunk: => A): IO[A] =
      IO.cede >> IO.pure(thunk) >> IO.cede // Not sure if delay + guarantee would be better.

  def attemptCompute[A](thunk: => Either[Throwable, A]): IO[A] =
    IO.cede >> IO.fromEither(thunk) >> IO.cede // ditto delay + guarantee.
}

These may be in another PR tho.
Especially since those should also be added to Async

Add two new combinators: cedeMap and intercede

ace83fc

armanbilge reviewed Jan 9, 2023

View reviewed changes

Add two new combinators: cedeMap and intercede

afe219a

armanbilge reviewed Jan 10, 2023

View reviewed changes

Add two new combinators: cedeMap and intercede

d6f9f68

armanbilge reviewed Jan 11, 2023

View reviewed changes

Add two new combinators: cedeMap and intercede

9b2ba64

armanbilge reviewed Jan 11, 2023

View reviewed changes

Add two new combinators: cedeMap and intercede

ed15013

armanbilge reviewed Jan 12, 2023

View reviewed changes

Add two new combinators: cedeMap and intercede

a5b8670

armanbilge approved these changes Jan 28, 2023

View reviewed changes

armanbilge added this to the v3.5.0 milestone Feb 4, 2023

djspiewak modified the milestones: v3.5.0, v3.6.0 Feb 14, 2023

BalmungSan reviewed Aug 11, 2023

View reviewed changes

djspiewak removed this from the v3.6.0 milestone Feb 19, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add two new combinators: `cedeMap` and `intercede` #3353

Add two new combinators: `cedeMap` and `intercede` #3353

biuld commented Jan 9, 2023

armanbilge Jan 9, 2023

biuld Jan 10, 2023

armanbilge Jan 10, 2023

armanbilge Jan 11, 2023

biuld Jan 11, 2023 •

edited

Loading

armanbilge Jan 11, 2023

biuld Jan 11, 2023 •

edited

Loading

armanbilge Jan 11, 2023

biuld Jan 11, 2023

armanbilge left a comment

armanbilge Jan 12, 2023

armanbilge Jan 12, 2023

armanbilge Jan 12, 2023

armanbilge Jan 12, 2023

biuld Jan 13, 2023 •

edited

Loading

armanbilge Jan 13, 2023

biuld Jan 14, 2023

armanbilge left a comment

djspiewak commented Feb 4, 2023

djspiewak commented Feb 14, 2023

BalmungSan left a comment •

edited

Loading

-  /**
-   * Functor map, but causes a reschedule before and after `f`
-   */
+  /**
+   * Like functor [[map]], but inserts a [[cede]] before and after `f`. Use this when `f` is a long-running, compute-bound operation.
+   *
+   * @see [[cede]] for more details
+   */

Add two new combinators: cedeMap and intercede #3353

Are you sure you want to change the base?

Add two new combinators: cedeMap and intercede #3353

Conversation

biuld commented Jan 9, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

biuld Jan 11, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

biuld Jan 11, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

armanbilge left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

biuld Jan 13, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

armanbilge left a comment

Choose a reason for hiding this comment

djspiewak commented Feb 4, 2023

djspiewak commented Feb 14, 2023

BalmungSan left a comment • edited Loading

Choose a reason for hiding this comment

Add two new combinators: `cedeMap` and `intercede` #3353

Add two new combinators: `cedeMap` and `intercede` #3353

biuld Jan 11, 2023 •

edited

Loading

biuld Jan 11, 2023 •

edited

Loading

biuld Jan 13, 2023 •

edited

Loading

BalmungSan left a comment •

edited

Loading