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

feat: Add support for for comprehensions. #935

Merged
merged 1 commit into from
Jan 13, 2024
Merged

Conversation

He-Pin
Copy link
Member

@He-Pin He-Pin commented Jan 13, 2024

Motivation:
refs: #654

Note:

  1. These methods are only added for scaladsl
  2. foreach method just work as fs2, where the Stream needs compile aka pekko's run. but different in ZIO, where foreach === runForeach.

Result:
for comprehensions works as fs2 for both Source and Flow in scaladsl

Copy link
Contributor

@pjfanning pjfanning left a comment

Choose a reason for hiding this comment

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

I would still like the new methods marked as experimental. I just think there are chances that we ay need to tweak this support if users try out for comprehensions and find some oddities.

@pjfanning
Copy link
Contributor

@pjfanning Do you mean add @ApiMayChange?

Maybe not that one - the API signatures won't change but we may need to tweak the internals of the functions.

Even we add something like this to the Scaladoc for the new functions.

Support for `for` comprehensions is experimental and it is possible that we might need to change the internal implementation to better support it.

@He-Pin
Copy link
Member Author

He-Pin commented Jan 13, 2024

I think the only in question is foreach, where ZIO alias to runForEach but fs2 alias to foreach and then needs a Stream#compile to run to a F[_], as Pekko stream always needs an explicitly run, so I think we can follow what fs2 does, and that's what @gmethvin once suggested too.

and for the withFilter and flatmMap , I think the currently shape should be fine, but I will update the scaladoc.

Copy link
Contributor

@pjfanning pjfanning left a comment

Choose a reason for hiding this comment

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

lgtm - I would like to see some other reviews though

@laglangyue
Copy link
Contributor

LGTM for code.
This is an Amazing feature, we shoudl update the document also,maybe in docs/src/main/paradox/stream/operators/Flow/ .

Copy link
Member

@jxnu-liguobin jxnu-liguobin left a comment

Choose a reason for hiding this comment

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

lgtm

@He-Pin
Copy link
Member Author

He-Pin commented Jan 13, 2024

Let me merge this, I think this one should be ready to go, any improvement can come up after.

@He-Pin He-Pin merged commit 72f0a42 into apache:main Jan 13, 2024
18 checks passed
@He-Pin He-Pin deleted the Stream-For branch January 13, 2024 16:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
t:stream Pekko Streams
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants