Introduce Mono#using{,When} Refaster rules
#1393
Conversation
|
Looks good. No mutations were possible for these changes. |
| D extends AutoCloseable, T, P extends Publisher<? extends T>, M extends Mono<? extends T>> { | ||
| @BeforeTemplate | ||
| Flux<T> before(Callable<D> resourceSupplier, Function<D, M> sourceSupplier) { | ||
| return Mono.using(resourceSupplier, sourceSupplier).flux(); |
There was a problem hiding this comment.
not sure I agree with this one; using Mono#flux at least makes explicit that it can emit at most 1 element (and sometimes a Flux is needed to adhere to method signatures)
the other way around is fine ofc. (Flux#single should generally only be used in specific edge cases, e.g. requesting 1 element from a batched endpoint)
There was a problem hiding this comment.
not sure I agree with this one; using Mono#flux at least makes explicit that it can emit at most 1 element (and sometimes a Flux is needed to adhere to method signatures)
If you check the method signature (Also what I was trying to explain here), is that the Function of the sourceSupplier explicitly returns a Mono, so by definition, we know the whole chain returns one element. So the Mono > Flux conversion seems redundant to me.
WDYT?
There was a problem hiding this comment.
Flux.using with a Mono sourceSupplier looks worse to me as IMO that just obfuscates what's going on, and thus I'd favour the explicit conversion with flux() (which is only needed in specific cases, for example adhering to a method return signature with multiple cases)
There was a problem hiding this comment.
Yeah, that makes sense to me 👌
Just to make sure I understand your stance, is this a "choosing a lesser evil" type of design choice? Or do you think that what we're attempting to re-write is actually a valid case?
There was a problem hiding this comment.
Or do you think that what we're attempting to re-write is actually a valid case
Yeah, I think Mono#flux should generally not be rewritten (bar some very specific exceptions where it's ultra-clear the Flux-variant only supplies 1 element, i.e. with just and error)
There was a problem hiding this comment.
Fair argument, I honestly have no strong opinion here.
Let's see what others think, then I will happily close the PR 😄 Thanks @Venorcis
There was a problem hiding this comment.
I think the other way around is very valid though 😄 i.e. don't use Flux#single when you can directly use a Mono instead
There was a problem hiding this comment.
..using Mono#flux at least makes explicit that it can emit at most 1 element ...
Though it is hard to know if the pattern was applied because it was intentionally specifying a flux that emits one element; or simply an unawareness of the equivalent APIs in Flux; Let's assume the former 👍
Applied ✅
There was a problem hiding this comment.
one could think of more very weird (but very straightforward) ones like:
Flux.error(...).single()/Mono.error(...).fluxFlux.empty().single()/Mono.empty().fluxFlux.just(...).single()/Mono.just(...).flux(not the Flux array signature ofc.)
and even most zip \ zipWith variants I guess
There was a problem hiding this comment.
Thanks for review @Venorcis 🙏
one could think of more very weird (but very straightforward) ones like:
Flux.error(...).single()/Mono.error(...).fluxFlux.empty().single()/Mono.empty().fluxFlux.just(...).single()/Mono.just(...).flux(not the Flux array signature ofc.)and even most
zip\zipWithvariants I guess
I agree. I wanted to limit the scope of this refaster rule to the #using API. Which started when I unknowingly had the same pattern in one of my PRs.
| D extends AutoCloseable, T, P extends Publisher<? extends T>, M extends Mono<? extends T>> { | ||
| @BeforeTemplate | ||
| Flux<T> before(Callable<D> resourceSupplier, Function<D, M> sourceSupplier) { | ||
| return Mono.using(resourceSupplier, sourceSupplier).flux(); |
There was a problem hiding this comment.
not sure I agree with this one; using Mono#flux at least makes explicit that it can emit at most 1 element (and sometimes a Flux is needed to adhere to method signatures)
If you check the method signature (Also what I was trying to explain here), is that the Function of the sourceSupplier explicitly returns a Mono, so by definition, we know the whole chain returns one element. So the Mono > Flux conversion seems redundant to me.
WDYT?
mohamedsamehsalah
force-pushed
the
mohamedsamehsalah/mono-using
branch
from
472962e to
49c3848
Compare
mohamedsamehsalah
changed the title
{Mono|Flux}#using{When} reactor refaster rulesMono#using{When} reactor refaster rules
|
Looks good. No mutations were possible for these changes. |
| D extends AutoCloseable, T, P extends Publisher<? extends T>, M extends Mono<? extends T>> { | ||
| @BeforeTemplate | ||
| Mono<T> before(Callable<D> resourceSupplier, Function<D, P> sourceSupplier) { | ||
| return Flux.using(resourceSupplier, sourceSupplier).single(); |
There was a problem hiding this comment.
(Here & below)
This refaster rule will cause a compiler error if the publisher in the supplier is not of type Mono. 😬
(Need to think how I can improve the matching here)
There was a problem hiding this comment.
Introduced a before template matcher here.
| D extends AutoCloseable, T, P extends Publisher<? extends T>, M extends Mono<? extends T>> { | ||
| @BeforeTemplate | ||
| Flux<T> before(Callable<D> resourceSupplier, Function<D, M> sourceSupplier) { | ||
| return Mono.using(resourceSupplier, sourceSupplier).flux(); |
There was a problem hiding this comment.
..using Mono#flux at least makes explicit that it can emit at most 1 element ...
Though it is hard to know if the pattern was applied because it was intentionally specifying a flux that emits one element; or simply an unawareness of the equivalent APIs in Flux; Let's assume the former 👍
Applied ✅
|
Looks good. All 7 mutations in this change were killed.
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed. |
|
Looks good. All 7 mutations in this change were killed.
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed. |
|
rickie
changed the title
Mono#using{When} reactor refaster rulesMono#using{,When} Refaster rules
Suggested commit message