scala-library-next
scala-library-next copied to clipboard
PoC: Add groupMapTo fluent API
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.
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?
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.
because it would allow calling
reduceafter specifying a customgroupsFactorywhich is not bad but feels like something a good API would disallow
Good point.
@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 😄
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 :+1: on the PR, or if you think we should not have it, vote with a :-1: