-
Notifications
You must be signed in to change notification settings - Fork 39
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
Introduce FluxMapNotNull{,Transformation}OrElse
Refaster rules
#1493
Conversation
Looks good. No mutations were possible for these changes. |
5821073
to
5cfb7ca
Compare
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.
Nice one @werli! I rebased and added a commit. Suggested commit message:
Introduce `FluxMapNotNull{,Transformation}OrElse` Refaster rules (#1493)
|
||
@BeforeTemplate | ||
Flux<S> before(Flux<T> flux) { | ||
return flux.map(x -> transformation(x)).mapNotNull(x -> x.orElse(null)); |
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.
It turns out that the test for this rule currently fails because x
is reused. Official Java scoping rules have these variables be independent, but I suspect that Refaster takes a shortcut somewhere. Good that we have tests :)
@@ -889,6 +889,35 @@ Flux<S> after(Flux<T> flux) { | |||
} | |||
} | |||
|
|||
/** Prefer {@link Flux#mapNotNull(Function)} over more contrived alternatives. */ |
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.
Both variants use mapNotNull
; will propose something.
@@ -889,6 +889,35 @@ Flux<S> after(Flux<T> flux) { | |||
} | |||
} | |||
|
|||
/** Prefer {@link Flux#mapNotNull(Function)} over more contrived alternatives. */ | |||
abstract static class FluxMapNotNullMapOptional<T, S> { |
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.
We're still figuring out our preferred naming strategy, but tentatively it'd be something like FluxMapNotNullOrElse
. Since the rule below would yield the same name, perhaps we'd go for FluxMapNotNullTransformationOrElse
here 🤔. Will use that for now.
FluxMapNotNull(Map)Optional
Refaster rulesFluxMapNotNull{,Transformation}OrElse
Refaster rules
Looks good. No mutations were possible for these changes. |
Quality Gate passedIssues Measures |
As @Stephan202 was already reviewing and I don't see any open points; I dropped the |
Thanks @Stephan202 & @rickie - LGTM 👍 |
I want to advocate for using
Flux#mapNotNull
more, even if that includes defaulting to an explicitnull
value.I'm happy to learn how to fuse those into one rule. 👀
Suggested commit message: