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 .singleOptional() to Mono and Flux #3317

Merged
merged 6 commits into from Jan 18, 2023

Conversation

AndreasHuber-CH
Copy link
Contributor

Fixes #3311

Signed-off-by: Andreas Huber andi@hubervonberg.ch

Fixes reactor#3311

Signed-off-by: Andreas Huber <andi@hubervonberg.ch>
@AndreasHuber-CH AndreasHuber-CH requested a review from a team as a code owner December 9, 2022 22:07
@AndreasHuber-CH
Copy link
Contributor Author

I've started off by duplicating the existing code for the single() use case and adapting it for the optional use case.
This allowed me also to copy over most unit tests.
But by doing that I ended up supporting Flux.singleOptional() use case as well.

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.
Would you prefer that?

@AndreasHuber-CH
Copy link
Contributor Author

Also, I haven't started designing marbles yet that stand for Optional.of() and Optional.empty(). Or could someone help me with that?

@chemicL
Copy link
Member

chemicL commented Jan 3, 2023

@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 the marble diagrams, I haven't created any yet so I'll get back to you once I clarify what the process is.
I'll go ahead and do a review once you remove the Flux implementations and we can follow up with the marbles after the initial review round.

@chemicL
Copy link
Member

chemicL commented Jan 3, 2023

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.

@AndreasHuber-CH
Copy link
Contributor Author

I removed the Flux.singleOptional() use case. The code is now ready to be reviewed.
For the svg, I'll try it out, but don't have a proposition yet.

Copy link
Member

@chemicL chemicL left a 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!

@chemicL
Copy link
Member

chemicL commented Jan 9, 2023

Also, please do validate the build result and run spotlessApply gradle job.

@AndreasHuber-CH
Copy link
Contributor Author

sorry about my last commit. It added useless whitespaces changes.
I imported the file https://github.com/reactor/reactor-core/blob/main/codequality/eclipse/reactor.style.xml into eclipse as I saw I was using "spaces" vs "tabs" used in other files. But it seems that messed up more than it fixed. Though in my local I didn't spot the newline changes... I'll revert

@AndreasHuber-CH
Copy link
Contributor Author

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.
(And I tried running it from WSL, but due to slower file access the command was taking forever)

@chemicL
Copy link
Member

chemicL commented Jan 16, 2023

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.
(And I tried running it from WSL, but due to slower file access the command was taking forever)

Not sure whether that can help (I'm not a Windows user), but perhaps a git configuration could fix such issues in the future:
https://community.openhab.org/t/spotless-converting-newlines-to-crlf/109493

Copy link
Member

@chemicL chemicL left a comment

Choose a reason for hiding this comment

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

LGTM!

@chemicL
Copy link
Member

chemicL commented Jan 16, 2023

I wrote a few JMH benchmarks to validate we indeed have a speedup over a combination of operators:

Benchmark                                          Mode  Cnt   Score   Error  Units
MonoOptionalBenchmark.singleOptional               avgt    5   4.284 ± 0.096  ns/op
MonoOptionalBenchmark.singleOptionalBaseline       avgt    5  38.576 ± 0.091  ns/op
MonoOptionalBenchmark.singleOptionalEmpty          avgt    5   0.591 ± 0.006  ns/op
MonoOptionalBenchmark.singleOptionalEmptyBaseline  avgt    5  32.916 ± 0.343  ns/op
MonoOptionalBenchmark.singleOptionalError          avgt    5  23.348 ± 0.165  ns/op
MonoOptionalBenchmark.singleOptionalErrorBaseline  avgt    5  59.782 ± 0.774  ns/op

Source: https://gist.github.com/chemicL/67b4f87a58950ae6e5d4dbcd4d2564b7

@OlegDokuka OlegDokuka added the type/enhancement A general enhancement label Jan 16, 2023
@OlegDokuka OlegDokuka added this to the 3.5.3 milestone Jan 16, 2023
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.
@chemicL chemicL merged commit 912441f into reactor:main Jan 18, 2023
@chemicL
Copy link
Member

chemicL commented Jan 18, 2023

Thank you for the contribution @AndreasHuber-CH! The PR is merged and will become part of the next release. 🚀

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.

Add method to transform Mono<T> into Mono<Optional<T>>
3 participants