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 groupMapReduce method to IterableOnceOps #23

Merged
merged 5 commits into from
Apr 5, 2021

Conversation

BalmungSan
Copy link
Contributor

@BalmungSan BalmungSan commented Oct 25, 2020

Fixes #8

I hope I correctly followed the structure and naming conventions that you want to enforce in this repo. I based my work here on what Seth is doing in #17 and I am aware that is also the first addition and that the guidelines may change, I subscribed to that PR to follow the discussion and conclusions (for example here I am following Sébastien advice on the naming of the implicit class), but if I miss something please let me know and I will happily push the requested changes.

Any kind of feedback is greatly welcome, but I am personally very interested in three things:

  1. Should the implicit class also be an AnyVal? I noticed Seth's one isn't and nobody discussed that. Is this just an oversight? Or personal preference? AFAIK for extension methods AnyVal should guarantee no instantiation and shouldn't have any funny side-effect since the class is not expected to be used as a type. But maybe I am missing something, maybe it has some problems with binary compatibility or something. Please let me know what you guys think about this.
  2. I basically copied the implementation from the one in IterableOps, with some sightly style changes... but I am worried those changes (especially using updateWith) could have a negative impact on performance.
  3. I am not sure if that test is enough but being honest I do not have any other ideas. Also, again I tried to copy what was on the stdlib, and this was the only test I found; but I won't say I read all the code, maybe I missed something?

Also, I would also like to propose adding groupByTo(factory)(key) and groupMapTo(factory)(key)(f) methods as well. What do you guys think? May I also add them in this PR or in another one? Or don't you find them useful (I do)?

@NthPortal
Copy link
Contributor

Should the implicit class also be an AnyVal?

yes

I noticed Seth's one isn't and nobody discussed that. Is this just an oversight?

possibly an oversight on Seth's part. I noticed it and planned to fix it or comment on it, but was waiting on a more concrete decision wrt package structure (see #24).

but I am worried those changes (especially using updateWith) could have a negative impact on performance.

I don't know if it will, but I'd probably just copy the implementation from the stdlib.

Also, I would also like to propose adding groupByTo(factory)(key) and groupMapTo(factory)(key)(f) methods as well.

sounds potentially valuable. I would recommend opening a separate ticket for that.

@BalmungSan BalmungSan changed the title Add group-like methods to Iterator Add groupMapReduce method to IterableOnceOps Oct 26, 2020
Copy link
Contributor

@NthPortal NthPortal left a comment

Choose a reason for hiding this comment

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

LGTM, excepting the one parameter change I noted.

I do want to make sure this is formally accepted before we merge though. It's not clear to me if the issue is considered an enhancement or bugfix, and if the former, it needs formal acceptance into the scala library

@BalmungSan
Copy link
Contributor Author

BalmungSan commented Nov 20, 2020

I do want to make sure this is formally accepted before we merge though.

@NthPortal Absolutely, as I said in #24 I believe here should only exist things that are already accepted to be part of a future Scala release.
I know that I made a risky move by implementing this before it was formally expressed it will be accepted, but I wanted to give it traction to this initiative.

I believe it would be worth adding a section in the README about when it is clear a change is to be accepted.
I would propose that a PR MUST always be linked to a previous issue and that the issue MUST be labelled ACCEPTED; or something like that.

@NthPortal
Copy link
Contributor

NthPortal commented Nov 20, 2020

I know that I made a risky move by implementing this before it was formally expressed it will be accepted, but I wanted to give it traction to this initiative.

Personally, I only think it is risky if you consider it to have been a significant amount of work. I agree that having an actual proposed/working implementation adds traction to it.

I would propose that a PR MUST always be linked to a previous issue and that the issue MUST be labelled ACCEPTED; or something like that.

I think we should narrow this slightly to things labeled enhancement, but on the whole I agree. I don't think things that are considered bugfixes or similar (e.g. the batchingGlobal ExecutionContext) that for technical reasons cannot be binary compatible should need formal acceptance though; I think their nature makes them "already accepted" so to speak.

That's actually why I commented about being unsure if this was considered an enhancement or bugfix.

Edit: I've executively decided it's an enhancement

@NthPortal NthPortal added enhancement New feature or request library:collections status:pending This enhancement request lacks consensus on whether or not to include it labels Nov 21, 2020
@BalmungSan
Copy link
Contributor Author

I just pushed the "parameter change" requested by @NthPortal as well as modifying the code so it follows the current most voted alternatives of the #24 & #46 proposals.

@NthPortal NthPortal merged commit 145ee2d into scala:main Apr 5, 2021
@NthPortal
Copy link
Contributor

I've merged this, because I think it's fairly uncontroversial. finalizing the style can come before a release

@BalmungSan BalmungSan deleted the add-iterator-group-methods branch April 5, 2021 22:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request library:collections status:pending This enhancement request lacks consensus on whether or not to include it
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add groupMapReduce to Iterator
3 participants