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.

Sign up for GitHub

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

Jump to bottom

feat: Add Sink#forall operator. #989

Merged
merged 1 commit into from
Jan 28, 2024
Merged

feat: Add Sink#forall operator. #989

merged 1 commit into from
Jan 28, 2024

Conversation

laglangyue
Copy link
Contributor

@laglangyue laglangyue commented Jan 20, 2024 •
edited by He-Pin
Loading

Motivation:
Add Sink#forall operator for to test whether all the elements match a predicate.
refs: #972

Note:
The origin author is @laglangyue and I updated and continued with it, so the review will leave to others.

image

@laglangyue laglangyue marked this pull request as draft January 20, 2024 09:27
@He-Pin
Copy link
Member

He-Pin commented Jan 20, 2024

@laglangyue Thanks and welcome the pr from China scala user group. ping me if you think it's ready.

@He-Pin
Copy link
Member

He-Pin commented Jan 20, 2024 •
edited
Loading

If you like , we can split the forAllAsync to a separate PR.
and for formating, you can use scala-cli fmt which is quit faster than the sbt scalafmtAll. And in for .md, will need an urlencoded for the scaladoc function signature.

@laglangyue
Copy link
Contributor Author

@laglangyue Thanks and welcome the pr from China scala user group. ping me if you think it's ready.

ok I will do this.

@He-Pin He-Pin added the t:stream Pekko Streams label Jan 20, 2024
@He-Pin He-Pin added this to the 1.1.0 milestone Jan 20, 2024
@laglangyue laglangyue marked this pull request as ready for review January 20, 2024 11:04
@laglangyue
Copy link
Contributor Author

@He-Pin ready

@laglangyue laglangyue marked this pull request as draft January 20, 2024 13:55
@laglangyue laglangyue marked this pull request as ready for review January 20, 2024 14:25
@laglangyue
Copy link
Contributor Author

laglangyue commented Jan 20, 2024 •
edited
Loading

I was so foolish. I actually implemented forall for the flow. I only found out after completing the test that the flow should not have forall. I remove them all.
It's ready now @He-Pin

@laglangyue laglangyue changed the title [pekko-streams] feat: add forall/forallAsync [pekko-streams] feat: add Sink.forall Jan 21, 2024
He-Pin
He-Pin reviewed Jan 21, 2024
View reviewed changes
Copy link
Member

@He-Pin He-Pin left a comment

Choose a reason for hiding this comment

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

Need some changes, I have leave the comments.

Roiocam
Roiocam reviewed Jan 22, 2024
View reviewed changes
Copy link
Member

@Roiocam Roiocam left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Maybe we could use the same approach as lightbend/sbt-paradox-apidoc#130 does?

docs/src/main/paradox/stream/operators/Sink/forall.md Outdated Show resolved Hide resolved
@laglangyue
Copy link
Contributor Author

This PR has been delayed for a week, and my father passed away due to cancer.

@He-Pin

This comment was marked as outdated.

Sign in to view
@He-Pin He-Pin changed the title [pekko-streams] feat: add Sink.forall feat: Add Sink#forall operator. Jan 24, 2024
@He-Pin He-Pin marked this pull request as ready for review January 24, 2024 18:06
@He-Pin He-Pin requested review from Roiocam and mdedetrich and removed request for Roiocam January 24, 2024 18:06
@He-Pin He-Pin mentioned this pull request Jan 24, 2024
Merged
@He-Pin He-Pin requested review from Roiocam and jxnu-liguobin and removed request for Roiocam January 26, 2024 07:46
He-Pin
He-Pin approved these changes Jan 27, 2024
View reviewed changes
Copy link
Member

@He-Pin He-Pin left a comment

Choose a reason for hiding this comment

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

lgtm, will need more eyes on this.

pjfanning
pjfanning requested changes Jan 27, 2024
View reviewed changes
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.

@since 1.1.0 on all new public methods

Roiocam
Roiocam approved these changes Jan 27, 2024
View reviewed changes
Copy link
Member

@Roiocam Roiocam left a comment

Choose a reason for hiding this comment

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

LGTM, Thanks.

I'm not an expert on the stream module, just some picky suggestions for paradox and testing.

docs/src/main/paradox/stream/operators/Sink/forall.md Outdated Show resolved Hide resolved
docs/src/test/java/jdocs/stream/operators/sink/ForAll.java Show resolved Hide resolved
docs/src/test/scala/docs/stream/operators/sink/ForAll.scala Show resolved Hide resolved
pjfanning
pjfanning approved these changes Jan 27, 2024
View reviewed changes
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

pjfanning
pjfanning requested changes Jan 27, 2024
View reviewed changes
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.

actually the since tags are still missing

@He-Pin

This comment was marked as outdated.

Sign in to view
@He-Pin He-Pin requested a review from pjfanning January 27, 2024 17:05
@He-Pin
Copy link
Member

He-Pin commented Jan 27, 2024

@pjfanning I think this one should be fine now, networking issue slow me done.

He-Pin
He-Pin reviewed Jan 27, 2024
View reviewed changes
stream-tests/src/test/scala/org/apache/pekko/stream/scaladsl/SinkSpec.scala

Source(1 to 2)
.runWith(sink)
.futureValue shouldBe true
Copy link
Member

Choose a reason for hiding this comment

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

@pjfanning The requested test have been added.

@He-Pin He-Pin linked an issue Jan 27, 2024 that may be closed by this pull request
Feature request:Add Sink/Flow.forall operator #972
Closed
@He-Pin
Copy link
Member

He-Pin commented Jan 28, 2024

@pjfanning @mdedetrich ping for a review

pjfanning
pjfanning approved these changes Jan 28, 2024
View reviewed changes
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

@He-Pin He-Pin merged commit f6da401 into apache:main Jan 28, 2024
18 checks passed
@pjfanning pjfanning added the release notes Need to release note label Feb 5, 2024
@pjfanning pjfanning removed the release notes Need to release note label Aug 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@He-Pin He-Pin He-Pin approved these changes

@pjfanning pjfanning pjfanning approved these changes

@Roiocam Roiocam Roiocam approved these changes

@nvollmar nvollmar Awaiting requested review from nvollmar

@mdedetrich mdedetrich Awaiting requested review from mdedetrich

@jxnu-liguobin jxnu-liguobin Awaiting requested review from jxnu-liguobin

Assignees
No one assigned
Labels
t:stream Pekko Streams
Projects
None yet
Milestone
1.1.0-M1
Development

Successfully merging this pull request may close these issues.

Feature request:Add Sink/Flow.forall operator
4 participants
@laglangyue @He-Pin @pjfanning @Roiocam