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.

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

Added flatMapN #4009

Merged
merged 5 commits into from
May 26, 2022
Merged

Added flatMapN #4009

merged 5 commits into from
May 26, 2022

Conversation

domaspoliakas
Copy link
Contributor

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

Comment on lines 525 to 555
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))"

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Comment on lines 379 to 382
val tfabc = mock[(F[A], F[B], F[C])]
val ff = mock[(A, B, C) => F[Z]]

tfabc.flatMapN(ff)
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

Yup, I think this is easy to add.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

@satorg satorg mentioned this pull request Feb 4, 2022
@armanbilge
Copy link
Member

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
Copy link
Contributor Author

I'll take another look when I can

@domaspoliakas
Copy link
Contributor Author

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

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

Choose a reason for hiding this comment

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

Suggested change
- /** @group MapArity */
- /** @group FlatMapArity */

Comment on lines +550 to +554
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)"
Copy link
Member

Choose a reason for hiding this comment

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

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 :)

Copy link
Member

Choose a reason for hiding this comment

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

Semigroupal captures the idea of composing independent effectful values.

Ah, ok that makes sense :)

armanbilge
armanbilge previously approved these changes May 18, 2022
Copy link
Member

@armanbilge armanbilge left a comment

Choose a reason for hiding this comment

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

Thank you very much, I think this looks good!

@armanbilge armanbilge added this to the 2.8.0 milestone May 18, 2022
armanbilge
armanbilge previously approved these changes May 23, 2022
@armanbilge armanbilge requested a review from satorg May 23, 2022 20:08
satorg
satorg previously approved these changes May 25, 2022
Copy link
Contributor

@satorg satorg left a comment

Choose a reason for hiding this comment

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

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

@domaspoliakas
Copy link
Contributor Author

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

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

@danicheg danicheg left a comment

Choose a reason for hiding this comment

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

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

Copy link
Member

@armanbilge armanbilge left a comment

Choose a reason for hiding this comment

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

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!

@armanbilge armanbilge requested a review from satorg May 26, 2022 13:37
@satorg
Copy link
Contributor

satorg commented May 26, 2022

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!

@armanbilge armanbilge merged commit 81bcdc8 into typelevel:main May 26, 2022
@domaspoliakas domaspoliakas deleted the feature/flatmappen branch May 26, 2022 15:02
@TonioGela TonioGela mentioned this pull request Jun 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Introducing .flatMapN(f) as a shorthand for .mapN(f).flatten
4 participants