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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Introduce MonoFromOptionalSwitchIfEmpty and OptionalMapMonoJust Refaster rules #384

Merged
merged 3 commits into from Dec 6, 2022
Merged

Introduce MonoFromOptionalSwitchIfEmpty and OptionalMapMonoJust Refaster rules #384

merged 3 commits into from Dec 6, 2022

Conversation

werli
Copy link
Member

@werli werli commented Dec 4, 2022 •
edited

I've spotted the following interesting pattern:

Optional.of(someNullableValue())
  .map(Mono::just)
  .orElse(someMono());

The mix between Optional and Mono API is unnecessary.

These rules aim to rewrite these to:

Mono.justOrEmpty(someNullableValue())
  .switchIfEmpty(someMono());

Suggested commit message:

Introduce `MonoFromOptionalSwitchIfEmpty` and `OptionalMapMonoJust` Refaster rules
(#384)

@github-actions
Copy link

github-actions bot commented Dec 4, 2022

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.

@Stephan202 Stephan202 added this to the 0.6.0 milestone Dec 4, 2022
Stephan202
Stephan202 approved these changes Dec 4, 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.

Thanks for opening a PR!

Added one commit :)

Tweaked suggested commit message (a bit long, but alas):

Introduce `OptionalMapMonoJust` and `MonoFromOptionalSwitchIfEmpty` Refaster rules
(#384)

error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ReactorRules.java Outdated
@BeforeTemplate
Mono<T> before(Optional<T> optional, Mono<T> alternative) {
return optional
.map(Refaster.<Function<T, Mono<T>>>anyOf(Mono::just, Mono::justOrEmpty))
Copy link
Member

Choose a reason for hiding this comment

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

The Mono::justOrEmpty -> Mono::just case can be its own "precursor rule", following our approach to defining the smallest improvement per Refaster rule. Will push a proposal.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good one, thanks. I didn't see that initially.

@github-actions
Copy link

github-actions bot commented Dec 4, 2022

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.

@werli werli mentioned this pull request Dec 4, 2022
Merged
rickie
rickie approved these changes Dec 4, 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.

Got one question.

Besides that, no remarks. LGTM!

W.r.t. suggested commit message, LGTM as well.

error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ReactorRules.java
}

/**
* Prefer a {@link Mono#justOrEmpty(Optional)} and {@link Mono#switchIfEmpty(Mono)} chain over
Copy link
Member

Choose a reason for hiding this comment

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

I know this is our usual way of defining the Javadoc.
However, it couldn't hurt to mention the exact reason either as @Description or instead of over more contrived alternatives. Not mixing the Optional and Mono API is a good argument and nice to document here as well.

We could IMO at least start with adding @Descriptions here and there.

Copy link
Member Author

Choose a reason for hiding this comment

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

I can try to come up with something later today. 🤞

Copy link
Member

Choose a reason for hiding this comment

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

We could IMO at least start with adding @Descriptions here and there.

I would defer that to a separate PR, as I'm still unsure about that feature. (Perhaps we can generate the @Description annotation at compile time, based on the Javadoc: that would make for a much nicer developer experience.)

Copy link
Member

Choose a reason for hiding this comment

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

Let's go with Javadoc then, deal?

Copy link
Member

Choose a reason for hiding this comment

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

Rebased and added a commit; PTAL.

Copy link
Member

Choose a reason for hiding this comment

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

LGTM!

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, would indeed feel like a different concern. Would have been weird starting this now in this PR. 🤔

@Stephan202 Stephan202 force-pushed the werli/mono-fromoptional-switchifempty branch from bdd5290 to 25cdb6d Compare December 5, 2022 16:49
@Stephan202
Copy link
Member

Unable to resolve action actions/setup-java@v3.7.0, unable to find version v3.7.0

Build fails due to actions/setup-java#422...

@rickie rickie force-pushed the werli/mono-fromoptional-switchifempty branch from 25cdb6d to 5eee6b5 Compare December 6, 2022 07:16
@github-actions
Copy link

github-actions bot commented Dec 6, 2022

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 title Introduce MonoFromOptionalSwitchIfEmpty Refaster rule Introduce MonoFromOptionalSwitchIfEmpty and OptionalMapMonoJust Refaster rules Dec 6, 2022
@rickie rickie merged commit ee62af4 into master Dec 6, 2022
@rickie rickie deleted the werli/mono-fromoptional-switchifempty branch December 6, 2022 07:26
@rickie
Copy link
Member

rickie commented Dec 6, 2022

Swapped the order of the names to make 'em sorted 😉.

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

@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
3 participants
@werli @Stephan202 @rickie