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 forEachAccumulating and refactor mapOrAccumulate #3367

Merged
merged 4 commits into from
Feb 6, 2024

Conversation

kyay10
Copy link
Collaborator

@kyay10 kyay10 commented Feb 2, 2024

First discussed on Slack.

This seems to be a useful primitive. I don't know whether exposing the iterator methods is a good idea or not. One can always use asSequence, but with extra wrapping.
I also added some missing mapOrAccumulate overloads that are refactored from other files.

Copy link
Contributor

github-actions bot commented Feb 2, 2024

Copy link
Member

@serras serras left a comment

Choose a reason for hiding this comment

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

I like the idea of this API, especially because then mapOrAccumulate becomes simply buildList { forEachOrAccumulate { ... } }. However, I think that the implementation is quite too convoluted compared to the previous one (I've left a suggestion on how to fix it).

@serras
Copy link
Member

serras commented Feb 2, 2024

I think that we should follow the convention from the rest of the functions, and call this forEachOrAccumulate instead (in line with zipOrAccumulate and mapOrAccumulate). WDYT @kyay10 @nomisRev ?

@serras
Copy link
Member

serras commented Feb 2, 2024

One can always use asSequence, but with extra wrapping.

Note that it's important to keep the semantics of Sequence, which is lazy evaluation of the elements. It's not the same to do sequence.toList().map(f).asSequence() that sequence.map(f); the latter won't start doing any work until the sequence is inspected, whereas the former will.

@kyay10
Copy link
Collaborator Author

kyay10 commented Feb 3, 2024

wrt forEachOrAccumulate: orXXX is usually used in the stdlib as some failing strategy with some normal return value. forEachAccumulating sounds more verb-like and hence imperative, which fits well with forEach. However, I see that consistency might be desireable here, so I'm fine with either name.

@kyay10
Copy link
Collaborator Author

kyay10 commented Feb 3, 2024

It's not the same to do sequence.toList().map(f).asSequence() that sequence.map(f); the latter won't start doing any work until the sequence is inspected, whereas the former will.

What I meant here was exposing forEachAccumulating(iterator: Iterator<A>). I left it exposed because it might be useful, but we might want to make it internal and instead users can use the sequence overload by wrapping their iterator in a sequence.

@kyay10 kyay10 requested a review from serras February 3, 2024 15:08
@kyay10
Copy link
Collaborator Author

kyay10 commented Feb 6, 2024

Quick note: I've used buildXXX extensively here, but I think there was a concern before that we should just use mutable collections to allow users to mutate if they really want to rather cheaply. Not sure If the take on that has changed, but just figured I should mention it

@serras serras merged commit 08f1540 into main Feb 6, 2024
11 checks passed
@serras serras mentioned this pull request Feb 8, 2024
22 tasks
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.

2 participants