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

Add F[TupleN] syntax #4125

Closed
wants to merge 3 commits into from
Closed

Add F[TupleN] syntax #4125

wants to merge 3 commits into from

Conversation

danicheg
Copy link
Member

@danicheg danicheg commented Feb 4, 2022

This adds convenient _1F..._22F methods for F[Tuple3..22]. At the moment, Cats contains those methods only for F[Tuple2], so it would be great to have it for all Tuple families.

I'm open to any suggestions/points about this.

@satorg
Copy link
Contributor

satorg commented Feb 4, 2022

As I understand, the worst concern regarding changes like this one is a potential increase of the result jar size.
Did you happen to estimate by chance how significantly the sizes got increased?

Link to a similar PR: #4009

@johnynek
Copy link
Contributor

johnynek commented Feb 4, 2022

this seems like a PR for mouse if you ask me:

https://github.com/typelevel/mouse

which, BTW, I would live to see merged into this repo since I believe cats is the only dependency. Then it would just be a submodule here.

@danicheg
Copy link
Member Author

danicheg commented Feb 4, 2022

@johnynek yeah, I was thinking about this, but it just confusing a bit to have F[Tuple2] syntax in the Cats and syntax for F[TupleN] in the Mouse (or have intersected on the same methods in both libraries).

@danicheg
Copy link
Member Author

danicheg commented Feb 8, 2022

Can we take yet another look at this? I have a concern about having this in the Mouse. It's just odd to have _1F, _2F for Tuple2 in Cats, but for TupleN in Mouse. But if it's common for all new syntaxes within cats.syntax, I doubtless will migrate this to Mouse.

@armanbilge
Copy link
Member

armanbilge commented Feb 8, 2022

It's just odd to have _1F, _2F for Tuple2 in Cats, but for TupleN in Mouse.

Yep, I agree with this. If this is the primary reason to target this PR to cats, maybe the real question is do _1F and _2F belong in Cats? IDK the history but maybe at the time they were added there was no mouse yet.

As I understand, the worst concern regarding changes like this one is a potential increase of the result jar size.
Did you happen to estimate by chance how significantly the sizes got increased?

Possibly off-topic, but I haven't been able to appreciate why this is such a big deal. See my comments in #3871 (comment). But I would like to know if there's something I'm not getting :)

@danicheg
Copy link
Member Author

danicheg commented Feb 8, 2022

IDK the history but maybe at the time they were added there was no mouse yet.

Mouse is about six years this May :) And _1F and _2F were added a year ago.

@armanbilge
Copy link
Member

I see, those methods were added in #3318.

@johnynek
Copy link
Contributor

johnynek commented Feb 9, 2022

I think in the past reviewers (such as me) did not think about mouse.

Adding 1F, 2F also can be related to the idea that tuple2 is special: we do have tuple2 FlatMap for instance. We might not necessarily have TupleN FlatMap.

I think considering Tuple2 as special, since it is kind of the primitive product type, but argue that in scala2, higher arities should go in mouse.

But it is hard to draw a line on how much we want to trade off jar size for functionality. It would be interesting to consider how we could have a framework for these kinds of PRs.

@danicheg
Copy link
Member Author

If we speak about the end-user experience it'd be totally bizarre to add a new library as a dependency to get syntaxes for TupleN (this and any future ones), as far I can see it. So if it's a clinched question for all these syntaxes, I won't make waves.

@danicheg danicheg closed this Feb 10, 2022
@danicheg danicheg deleted the ftuple-syntax branch February 10, 2022 15:25
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.

4 participants