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

PoC: Add groupMapTo fluent API #53

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

BalmungSan
Copy link
Contributor

@BalmungSan BalmungSan commented Dec 7, 2020

Fixes #51

I know there are a lot of things to discuss like the name of the methods, visibility constraints, where to place everything, etc.
But for now, I want to focus the discussion on if this core PoC is generally well perceived by maintainers.

}

object NextIterableOnceOpsExtensions {
final case class GroupMap[A, K, V, CC[_], MC[_, _]](
Copy link
Contributor

Choose a reason for hiding this comment

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

if we plan this to be in a future Scala version, it should probably be outside of the next package

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah as I said, if this PoC is approved then we need to discuss where to put everything and which accessibility scope they should have.

@BalmungSan
Copy link
Contributor Author

Hi @NthPortal @julienrf

I changed the design, this new version should allow all the possible combinations without problems and allowing it to be used by all collection types.

Now I am missing docs and more tests but before that, I would like to ask for suggestions on the names of the methods / classes; because being honest I do not like any of them (especially the GenGen ones).
But, even before that, it would be good to know what do you guys think about this proposal? Does it have future? Or is this something that the stdlib will never have?

Copy link
Contributor

@julienrf julienrf 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 @BalmungSan for your continuous effort!

My opinion is that I think this feature is useful, and it’s something difficult to achieve with the current API of the standard library, so I think it’s a good candidate for scala-library-next.

I agree that the naming is not great 😄 The Gen suffix does not mean something clear to me. And GenGen is even more confusing.

The current way to apply operations to collections without evaluating their elements is to use views, so I wonder if we could reuse this vocabulary here in place of “gen” (see my suggestions below…). Also, it seems that we could unify GroupMapGen and GroupMapGenGen, since GroupMapGen is a GroupMapGenGen with a fixed valuesFactory.

@BalmungSan
Copy link
Contributor Author

Hi @julienrf thanks for the naming suggestions, I like them a lot!

The current way to apply operations to collections without evaluating their elements is to use views

I actually spent some time thinking if all this could be replaced with a current view but found it difficult, did not realize that I could have used that name!

Also, it seems that we could unify GroupMapGen and GroupMapGenGen, since GroupMapGen is a GroupMapGenGen with a fixed valuesFactory.

That was the original design, I decided to split it because keeping them unified had the consequence of adding an implicit parameter to all the group* methods (although that could be fixed when merging this with the real stdlib code by requiring those factories as members of IterableOnceOps as they are on IterableOps) and because it would allow calling reduce after specifying a custom groupsFactory which is not bad but feels like something a good API would disallow. What do you think?
Now, a good point of having them unified is that we do not need to think a second name and we can reduce some of the additional methods.

@julienrf
Copy link
Contributor

because it would allow calling reduce after specifying a custom groupsFactory which is not bad but feels like something a good API would disallow

Good point.

@BalmungSan
Copy link
Contributor Author

@julienrf thanks again for the review and the great naming suggestions, I have just pushed them.

Now, again, this is missing docs and probably more tests. But I would rather not do all that work until it is clear if this would be included or not; so I will wait until then. In any case, if you (or anyone) have more suggestions or want to discuss something about the design I would be more than happy to reply 😄

@julienrf
Copy link
Contributor

I do believe this is a useful addition to the standard library, but we may need to do a few more passes on the design.

Please, if anyone else thinks it’s a valuable addition vote with a 👍 on the PR, or if you think we should not have it, vote with a 👎

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.

Add groupByTo and groupMapTo to IterableOnceOps
3 participants