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.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Introduce {Mono,Flux}DefaultIfEmpty Refaster rules #370

Merged
merged 5 commits into from
Nov 28, 2022
Merged

Introduce {Mono,Flux}DefaultIfEmpty Refaster rules #370

merged 5 commits into from
Nov 28, 2022

Conversation

giall
Copy link
Contributor

@giall giall commented Nov 25, 2022

Saw "good first issue" and could not help myself 馃槃

Issue: #363

Would like some suggestions on the rule names, not too happy with FluxSwitchIfEmptyOfMonoOrFluxJust especially.

@giall giall mentioned this pull request Nov 25, 2022
Closed
3 tasks
@github-actions
Copy link

Looks good. No mutations were possible for these changes.
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

2 similar comments
@github-actions
Copy link

Looks good. No mutations were possible for these changes.
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

@github-actions
Copy link

Looks good. No mutations were possible for these changes.
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

@rickie rickie changed the base branch from sschroevers/configure-arcmutate to master November 26, 2022 09:18
@rickie rickie added this to the 0.6.0 milestone Nov 26, 2022
@rickie
Copy link
Member

rickie commented Nov 26, 2022

Hey @giall ! Nice that you opened the PR!

I forgot to update the base branch back to master 馃槃. (Nice to see that the reporting of Pitest works though).

Stephan202
Stephan202 reviewed Nov 26, 2022
View reviewed changes
Copy link
Member

@Stephan202 Stephan202 left a comment

Choose a reason for hiding this comment

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

Added a commit.

error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ReactorRules.java Outdated
@@ -264,6 +264,32 @@ Flux<T> after(Flux<T> flux, long n) {
}
}

/** Prefer {@link Mono#defaultIfEmpty(Object)} over more contrived alternatives. */
static final class MonoSwitchIfEmptyOfMonoJust<T> {
Copy link
Member

Choose a reason for hiding this comment

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

Since we name the rules after the preferred expression:

Suggested change
static final class MonoSwitchIfEmptyOfMonoJust<T> {
static final class MonoDefaultIfEmpty<T> {

error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ReactorRules.java
/** Prefer {@link Mono#defaultIfEmpty(Object)} over more contrived alternatives. */
static final class MonoSwitchIfEmptyOfMonoJust<T> {
@BeforeTemplate
Mono<T> before(Mono<T> mono, T object) {
Copy link
Member

Choose a reason for hiding this comment

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

object may be a subtype of T, but I see that in either case Refaster matches correctly (didn't commit the test changes, but the point is that this expression is rewritten as well:

Flux.<Object>just("X").switchIfEmpty(Mono.just(1))

@Stephan202
Copy link
Member

Suggested commit message:

Introduce `{Mono,Flux}DefaultIfEmpty` Refaster rules (#370)

Resolves #363.

werli
werli approved these changes Nov 27, 2022
View reviewed changes
Copy link
Member

@werli werli left a comment

Choose a reason for hiding this comment

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

Very nice @giall 馃挭

EnricSala
EnricSala approved these changes Nov 28, 2022
View reviewed changes
Copy link

@EnricSala EnricSala 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 picking this up :)

rickie
rickie approved these changes Nov 28, 2022
View reviewed changes
Copy link
Member

@rickie rickie left a comment

Choose a reason for hiding this comment

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

Nice @giall congrats on your first PR 馃殌 !

Suggested commit message LGTM!

@rickie rickie force-pushed the giall/mono_switch_if_empty_of_mono_just branch from 1c3d64e to 27a5d01 Compare November 28, 2022 11:57
@rickie
Copy link
Member

rickie commented Nov 28, 2022

Rebased. Will merge once 馃煝.

@rickie rickie force-pushed the giall/mono_switch_if_empty_of_mono_just branch from 27a5d01 to 4baeda5 Compare November 28, 2022 12:01
@rickie rickie changed the title Simplify Reactor switchIfEmpty({Mono|Flux}.just(...)) using defaultIfEmpty Introduce {Mono,Flux}DefaultIfEmpty Refaster rules Nov 28, 2022
@rickie rickie merged commit 6d15cfe into master Nov 28, 2022
@rickie rickie deleted the giall/mono_switch_if_empty_of_mono_just branch November 28, 2022 12:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Stephan202 Stephan202 Stephan202 approved these changes

@EnricSala EnricSala EnricSala approved these changes

@werli werli werli approved these changes

@rickie rickie rickie approved these changes

Assignees
No one assigned
Labels
new feature
Milestone
0.6.0
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@giall @rickie @Stephan202 @EnricSala @werli