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

completed SwitchOnFirst impl #1423

Merged
merged 20 commits into from
Dec 19, 2018
Merged

Conversation

OlegDokuka
Copy link
Contributor

porting of the SwitchOnFirst operator's implementation with no fusion for now. This is a complete, tested, merge-ready implementation. Fuseable part will be submitted separately since it is a little bit tricky

@codecov-io
Copy link

codecov-io commented Nov 8, 2018

Codecov Report

Merging #1423 into master will decrease coverage by 0.09%.
The diff coverage is 73.41%.

Impacted file tree graph

@@             Coverage Diff             @@
##             master    #1423     +/-   ##
===========================================
- Coverage     84.27%   84.18%   -0.1%     
- Complexity     3895     3910     +15     
===========================================
  Files           358      359      +1     
  Lines         29435    29760    +325     
  Branches       5464     5527     +63     
===========================================
+ Hits          24806    25052    +246     
- Misses         3030     3078     +48     
- Partials       1599     1630     +31
Impacted Files Coverage Δ Complexity Δ
...ore/src/main/java/reactor/core/publisher/Flux.java 98.73% <100%> (ø) 511 <1> (+1) ⬆️
...rc/main/java/reactor/core/publisher/Operators.java 79.79% <63.15%> (-0.33%) 120 <2> (+3)
...java/reactor/core/publisher/FluxSwitchOnFirst.java 73.98% <73.98%> (ø) 3 <3> (?)
.../java/reactor/core/publisher/BlockingIterable.java 77.08% <0%> (-2.09%) 7% <0%> (ø)
...ava/reactor/core/publisher/WorkQueueProcessor.java 69.42% <0%> (-1.66%) 19% <0%> (ø)
.../java/reactor/util/concurrent/MpscLinkedQueue.java 88.15% <0%> (-1.32%) 17% <0%> (-1%)
...r-core/src/main/java/reactor/core/Disposables.java 88.95% <0%> (-1.23%) 21% <0%> (ø)
...java/reactor/core/publisher/FluxBufferTimeout.java 77.85% <0%> (-0.68%) 3% <0%> (ø)
...c/main/java/reactor/core/publisher/FluxExpand.java 94.61% <0%> (-0.45%) 3% <0%> (ø)
...ain/java/reactor/core/publisher/FluxPublishOn.java 86.59% <0%> (-0.21%) 6% <0%> (ø)
... and 19 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4f9e176...7003ffc. Read the comment docs.

@simonbasle
Copy link
Member

@OlegDokuka we'll hopefully have time to review in depth for 3.2.4 release, won't be able to in time for thursday's 3.2.3, but we haven't forgotten about this PR 😉

@OlegDokuka
Copy link
Contributor Author

@OlegDokuka we'll hopefully have time to review in depth for 3.2.4 release, won't be able to in time for thursday's 3.2.3, but we haven't forgotten about this PR 😉

Hope this time it will not be like with AsyncFileFlux ;)

@bsideup
Copy link
Contributor

bsideup commented Nov 26, 2018

Since it took a bit of time to understand this operator, here is a link to the original PR where this operator was added to rsocket-java:
rsocket/rsocket-java#435 (comment)

I find the description quite informative:

Added SwitchTransform to internal, useful when you need to look at the first message on a channel before deciding how to handle it.

@simonbasle
Copy link
Member

please add more commit as you fix the review elements @OlegDokuka, we'll rebase/squash in the final step (or I'll squash when merging)

Copy link
Member

@simonbasle simonbasle left a comment

Choose a reason for hiding this comment

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

Thanks for the changes! I found another small test issue (assumption instead of assertion), plus it is still missing some documentation and a test for toConditionalSubscriber, otherwise LGTM

@OlegDokuka
Copy link
Contributor Author

@simonbasle WIP. I'm traveling right now, so trying to push commits in transition 😄

@OlegDokuka OlegDokuka closed this Dec 17, 2018
@OlegDokuka OlegDokuka reopened this Dec 17, 2018
Copy link
Member

@simonbasle simonbasle left a comment

Choose a reason for hiding this comment

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

please don't include a marble yet, there is an upcoming PR making contribution of marbles much easier and much more likely to produce similar-looking marbles.

Can you please remove both image files from this PR and add the operator to the list of missing SVG (see comment on the javadoc URL)?

Other issues listed in comments.

@simonbasle simonbasle merged commit 1230bdd into reactor:master Dec 19, 2018
@simonbasle simonbasle added this to the 3.2.4.RELEASE milestone Dec 19, 2018
@simonbasle simonbasle added the type/enhancement A general enhancement label Dec 19, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type/enhancement A general enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants