Added flatMapN by domaspoliakas · Pull Request #4009 · typelevel/cats

domaspoliakas · 2021-10-06T12:14:22Z

Hello 👋

As promised here is .flatMapN. Took me a little while to get my head around Boilerplate. Let me know if this works for you, or if you need anything else.

resolves #4003

satorg · 2021-10-09T07:07:58Z

project/Boilerplate.scala

+      val flatMap =
+        if (arity == 1)
+          s"def flatMap[Z](f: (${`A..N`}) => F[Z])(implicit flatMap: FlatMap[F]): F[Z] = flatMap.flatMap($tupleArgs)(f)"
+        else
+          s"def flatMapN[Z](f: (${`A..N`}) => F[Z])(implicit flatMap: FlatMap[F], semigroupal: Semigroupal[F]): F[Z] = flatMap.flatten(Semigroupal.map$arity($tupleArgs)(f))"
+


Looking into the Boilerplate code I noticed, that support for mapN is currently implemented in this way:

SemigroupalArityFunctions contains a bunch of mapX (2 ≤ X ≤ 22) functions (along with other arity functions like imapX and contramapX.

These implementations are used then from all other arity syntax, including TupleSemigroupalSyntax.

Shouldn't we follow the same pattern for flatMapN – i.e. add implementations for all flatMapX into SemigroupalArityFunctions in the first place and then make use of it from everywhere else?

I am not certain about it though, just an observation.

Also there's the ApplyArityFunctions trait that adds the mapX syntax directly to the Apply typeclass.

Curious, shouldn't we create a FlatMapArityFunctions trait (it does not exist yet) and add flatMapX method directly into the FlatMap typeclass as well?

All excellent questions! My reason for doing it as it is is the number of methods. The initial concern with regards to adding .flatMapN was jar size, since adding arity methods adds a lot of code. Because of that I opted to only add it where it's - in my opinion - most useful, which is the tuple syntax. I personally have never used Apply[F].mapN, so I don't know if it's particularly useful to add a FlatMap[F].flatMapN. But if the consensus says otherwise I'm happy to change that.

Agree, jar size is a concern. Did you try by chance to measure how much the resulting jars get increased in size comparing to the base version? May be it adds not too much...

I did not actually. I'll give it a shot today/tomorrow and see how it compares.

satorg · 2021-10-09T19:05:09Z

tests/src/test/scala/cats/tests/SyntaxSuite.scala

+    val tfabc = mock[(F[A], F[B], F[C])]
+    val ff = mock[(A, B, C) => F[Z]]
+
+    tfabc.flatMapN(ff)


Tests are always good, I personally appreciate it. The only notice: there's an if condition in your code, but this test only checks a branch for arity > 1. Although I don't think it is absolutely necessary, but it would be nice if you could add a test for the arity = 1 branch.

Yup, I think this is easy to add.

I would really like to have this addressed (since the syntax for Tuple1 seems to be slightly different), but I'm not going to hold the entire PR on just because of this concern only.

armanbilge · 2022-05-16T22:40:22Z

Personally, I frequently find myself wanting flatMapN. Also, IMHO jar size is not that big of a deal, see commentary in #3871 (comment).

In theory, we add methods to typeclasses (and syntax merely delegates to those) so that instances can override those with more efficient implementations (you can't override syntax extensions :)

In practice, not sure if it makes a meaningful difference here. But still I think that adding these methods to a FlatMapArityFunctions would probably be the way to do it.

domaspoliakas · 2022-05-17T08:14:23Z

I'll take another look when I can

domaspoliakas · 2022-05-18T11:07:52Z

Rebased on latest main, added FlatMapArityFunctions and made tuple syntax delegate to those instead of the previous implementation

armanbilge · 2022-05-18T15:24:00Z

project/Boilerplate.scala

+      | * @groupprio FlatMapArity 999
+      | */
+      |trait FlatMapArityFunctions[F[_]] { self: FlatMap[F] =>
+        -  /** @group MapArity */


Suggested change

- /** @group MapArity */

- /** @group FlatMapArity */

armanbilge · 2022-05-18T15:31:27Z

project/Boilerplate.scala

+      val flatMap =
+        if (arity == 1)
+          s"def flatMap[Z](f: (${`A..N`}) => F[Z])(implicit flatMap: FlatMap[F]): F[Z] = flatMap.flatMap($tupleArgs)(f)"
+        else
+          s"def flatMapN[Z](f: (${`A..N`}) => F[Z])(implicit flatMap: FlatMap[F]): F[Z] = flatMap.flatMap$arity($tupleArgs)(f)"


So I haven't been able to wrap my head around whether this technically belongs in TupleSemigroupalSyntax. It's certainly the easiest place to put it, and it works, I'm just confused by the "semigroupal" in the name :)

Semigroupal captures the idea of composing independent effectful values.

Ah, ok that makes sense :)

armanbilge

Thank you very much, I think this looks good!

satorg

lgtm, thanks a lot. The test for the arity = 1 case would be appreciated though.

domaspoliakas · 2022-05-26T08:38:54Z

I'll add the test, but I'm honestly not sure why there even is a Tuple1 syntax. Is that useful?

danicheg

I was dreaming of flatMapN for years. It's nice to see this happening!

armanbilge

I'll add the test, but I'm honestly not sure why there even is a Tuple1 syntax. Is that useful?

Agree, pretty dubious about this 😆 but it's good for completeness. Thanks!

satorg · 2022-05-26T14:51:51Z

I'll add the test, but I'm honestly not sure why there even is a Tuple1 syntax. Is that useful?

Perhaps just for completeness and consistency with other syntax, I guess.
Thank you, looks really great!

satorg reviewed Oct 9, 2021

View reviewed changes

satorg mentioned this pull request Feb 4, 2022

Add F[TupleN] syntax #4125

Closed

Added flatMapN

af65abb

domaspoliakas force-pushed the feature/flatmappen branch from 15a9806 to 89f1bc0 Compare May 18, 2022 11:07

Added FlatMapArityFunctions, delegated flatMapN to those

980c53e

domaspoliakas force-pushed the feature/flatmappen branch from 89f1bc0 to 980c53e Compare May 18, 2022 11:53

armanbilge reviewed May 18, 2022

View reviewed changes

Fixed typo in FlatMapArityFunctions

6a7ddc0

armanbilge reviewed May 18, 2022

View reviewed changes

armanbilge previously approved these changes May 18, 2022

View reviewed changes

armanbilge added this to the 2.8.0 milestone May 18, 2022

Merge branch 'main' into feature/flatmappen

8631a49

armanbilge dismissed their stale review via 8631a49 May 23, 2022 20:07

armanbilge previously approved these changes May 23, 2022

View reviewed changes

armanbilge requested a review from satorg May 23, 2022 20:08

satorg previously approved these changes May 25, 2022

View reviewed changes

Added tuple1 flatMap syntax test

f57146a

domaspoliakas dismissed stale reviews from satorg and armanbilge via f57146a May 26, 2022 08:48

danicheg approved these changes May 26, 2022

View reviewed changes

armanbilge approved these changes May 26, 2022

View reviewed changes

armanbilge requested a review from satorg May 26, 2022 13:37

satorg approved these changes May 26, 2022

View reviewed changes

armanbilge merged commit 81bcdc8 into typelevel:main May 26, 2022

domaspoliakas deleted the feature/flatmappen branch May 26, 2022 15:02

TonioGela mentioned this pull request Jun 17, 2022

Added parFlatMapN #4243

Merged

Uh oh!

Conversation

domaspoliakas commented Oct 6, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

armanbilge commented May 16, 2022

Uh oh!

domaspoliakas commented May 17, 2022

Uh oh!

domaspoliakas commented May 18, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

armanbilge left a comment

Choose a reason for hiding this comment

Uh oh!

satorg left a comment

Choose a reason for hiding this comment

Uh oh!

domaspoliakas commented May 26, 2022

Uh oh!

danicheg left a comment

Choose a reason for hiding this comment

Uh oh!

armanbilge left a comment

Choose a reason for hiding this comment

Uh oh!

satorg commented May 26, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants