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 mapAccumulate to Observable #1065

Merged
merged 5 commits into from Nov 5, 2019

Conversation

@busti
Copy link
Contributor

busti commented Oct 31, 2019

This Pull Request adds support for the following operator:

def mapAccumulate[S, R](seed: => S)(op: (S, A) => (S, R)): Observable[R]

It is similar to scan with the difference being that it allows for a separate accumulator and result type.
It is analogous to the mapAccumulate operator found in fs2.

I am still uncertain about the naming and the placement of the operator in context to the other operators. The naming in fs2 implies that it is more closely related to map, however I personally think that it is more similar to scan. I initially called it scanTo which is a bit shorter and maybe clearer.
Also, scanAccumulate might be redundant, since scan already implies that an accumulation is happening.

@Avasil
Avasil approved these changes Nov 3, 2019
@Avasil

This comment has been minimized.

Copy link
Collaborator

Avasil commented Nov 3, 2019

Thank you @busti , the implementation looks good to me

The naming in fs2 implies that it is more closely related to map, however I personally think that it is more similar to scan. I initially called it scanTo which is a bit shorter and maybe clearer.
Also, scanAccumulate might be redundant, since scan already implies that an accumulation is happening.

I agree and I think we should go with either scanTo or mapAccumulate.
I know that ZIO also use mapAccum so maybe the method has a bigger history. I asked on fs2 channel if they know the origins of the name.

@Avasil

This comment has been minimized.

Copy link
Collaborator

Avasil commented Nov 4, 2019

@busti It seems like the name is derived from Haskell's mapAccumL with similar signature:

functional-streams-for-scala/fs2#309

http://hackage.haskell.org/package/base-4.12.0.0/docs/Data-Traversable.html

@busti

This comment has been minimized.

Copy link
Contributor Author

busti commented Nov 4, 2019

Thanks a lot for looking further into that. Then I see no reason why we should not name it like that either. I am personally in favor of mapAccum since it is a bit shorter. Thoughts?
I can make these changes this evening once I get back from work.

@Avasil

This comment has been minimized.

Copy link
Collaborator

Avasil commented Nov 4, 2019

I'm leaning towards mapAccumulate because I don't see any abbreviated names in Observable but I don't have strong opinion either way so go with what you prefer :)

cc @oleg-py if he wants to join the bikeshedding :D

@busti

This comment has been minimized.

Copy link
Contributor Author

busti commented Nov 4, 2019

because I don't see any abbreviated names in Observable

That makes perfect sense. Sorry for the unnecessary bikeshedding :shipit:

@busti busti changed the title Add scanAccumulate to Observable Add mapAccumulate to Observable Nov 5, 2019
@busti

This comment has been minimized.

Copy link
Contributor Author

busti commented Nov 5, 2019

I changed the name, but it is still placed underneath scan.
(more bikeshedding incoming) Since the functionality and implementation is almost exactly as the one of scan, I would personally leave it there.
When I had to order these functions in the collection library that I am working on at the moment, I simply ordered them alphabetically.

@Avasil

This comment has been minimized.

Copy link
Collaborator

Avasil commented Nov 5, 2019

I think the placement is alright. In any case it is something that can be easily changed :)

Thank you for the contribution!

@Avasil Avasil merged commit 6822011 into monix:master Nov 5, 2019
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@busti busti deleted the busti:scanAccumulate branch Nov 6, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.