-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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 .singleOptional() to Mono and Flux #3317
Conversation
Fixes reactor#3311 Signed-off-by: Andreas Huber <[email protected]>
I've started off by duplicating the existing code for the single() use case and adapting it for the optional use case. If you prefer not including the Flux part, I can delete the Flux.singleOptional(), move corresponding code from the MonoSingleOptional to MonoSingleOptionalMono, and the same for unit tests. |
Also, I haven't started designing marbles yet that stand for Optional.of() and Optional.empty(). Or could someone help me with that? |
@AndreasHuber-CH coming back after a break - thanks for the PR! Unless you have a justified use case for Flux.singleOptional(), please remove the implementation. In my head, Flux.collectList() is the logical counterpart for a sequence of items. If it's needed, we can add it in the future. |
For marble diagrams, it occurs the team has been using Inkscape and more detailed instructions are in https://github.com/reactor/reactor-core/tree/main/docs/svg - the conventions.svg file can be used as the source of visual components for creating a new marble diagram. |
I removed the Flux.singleOptional() use case. The code is now ready to be reviewed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the changes. I did review the code and left a few comments to address :) Good luck with the marble diagram, hope it works out!
reactor-core/src/main/java/reactor/core/publisher/MonoSingleOptional.java
Outdated
Show resolved
Hide resolved
reactor-core/src/main/java/reactor/core/publisher/MonoSingleOptional.java
Outdated
Show resolved
Hide resolved
reactor-core/src/test/java/reactor/core/publisher/MonoSingleOptionalTest.java
Outdated
Show resolved
Hide resolved
reactor-core/src/main/java/reactor/core/publisher/MonoSingleOptionalCallable.java
Show resolved
Hide resolved
reactor-core/src/main/java/reactor/core/publisher/MonoSingleOptional.java
Outdated
Show resolved
Hide resolved
Also, please do validate the build result and run spotlessApply gradle job. |
sorry about my last commit. It added useless whitespaces changes. |
c8520fe
to
e53048c
Compare
e53048c
to
3c61b60
Compare
OK I fixed the windows line ending issue. Actually it wasn't the eclipse formatter, but when "gradlew spotlessApply" is run on Windows, it changes line endings of all files it updates from Linux to Windows line endings. |
Not sure whether that can help (I'm not a Windows user), but perhaps a git configuration could fix such issues in the future: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
I wrote a few JMH benchmarks to validate we indeed have a speedup over a combination of operators:
Source: https://gist.github.com/chemicL/67b4f87a58950ae6e5d4dbcd4d2564b7 |
This simplifies the MonoSingleOptional class. In case onNext is called multiple times (which should not occur for Mono anyway) extranoues elements are simply dropped rather than throwing IndexOutOfBoundsException.
reactor-core/src/main/java/reactor/core/publisher/MonoSingleOptional.java
Show resolved
Hide resolved
reactor-core/src/main/java/reactor/core/publisher/MonoSingleOptional.java
Show resolved
Hide resolved
Thank you for the contribution @AndreasHuber-CH! The PR is merged and will become part of the next release. 🚀 |
Fixes #3311
Signed-off-by: Andreas Huber [email protected]