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

Introduce additional Reactor Refaster rules #969

Merged
merged 3 commits into from
Jan 21, 2024

Conversation

Stephan202
Copy link
Member

@Stephan202 Stephan202 commented Jan 13, 2024

Suggested commit message:

Introduce additional Reactor Refaster rules (#969)

In various contexts, suggest more efficient and/or less verbose
constructs.

@Stephan202 Stephan202 added this to the 0.15.0 milestone Jan 13, 2024
@Stephan202 Stephan202 requested review from werli and rickie January 13, 2024 14:58
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.

@Stephan202 Stephan202 force-pushed the sschroevers/additional-reactor-refaster-rules branch from a8447b0 to f12c569 Compare January 13, 2024 20:23
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.

Cool stuff


@AfterTemplate
Mono<T> after(Flux<T> flux) {
return flux.transform(MathFlux::max).next();
Copy link
Member

Choose a reason for hiding this comment

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

Food for thought: MathFlux#max returns a Mono, so the resulting Flux has either zero or one element. Should we make this more explicit but using Flux#singleOrEmpty()? This comment applies for more parts of the PR.

Suggested change
return flux.transform(MathFlux::max).next();
return flux.transform(MathFlux::max).singleOrEmpty();

Copy link
Member Author

Choose a reason for hiding this comment

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

Nice suggestion; applied!

Copy link
Member

Choose a reason for hiding this comment

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

Maybe this opens up a box of other possible improvements.

Any conversion Mono -> Flux -> Mono for which we know at most one element emitted and that's currently using Flux#next should be rewritten? E.g. flux.transform(xyz -> mono).next() could call Flux#singleOrEmpty().

Copy link
Member Author

@Stephan202 Stephan202 Jan 15, 2024

Choose a reason for hiding this comment

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

Indeed. There's a whole class of "replace operator based on an invariant established by preceding operators" rewrite rules, where these invariants are often of the form "between x and y elements will be emitted". In some cases it may be more efficient/practical to introduce an Error Prone check for that, like we did for NonEmptyMono.

I was planning to add a commit with two more rules:

  • One that matches your suggestion, with a note to generalize it later.
  • Another that I came up with when writing a test case for the first. This one cleans up a pattern found in our codebase.

(The idea being that this is a lightweight approach to tracking these ideas.)

... however, the first of these rules doesn't work, due to what appears to be a bug in Refaster's matching of generic types. I previously hit a similar issue, so I added this rule to another branch.

@Stephan202 Stephan202 force-pushed the sschroevers/additional-reactor-refaster-rules branch from f12c569 to 015c221 Compare January 15, 2024 09:56
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.

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.

Copy link
Contributor

@Venorcis Venorcis left a comment

Choose a reason for hiding this comment

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

LGTM 👍🏻

@rickie rickie force-pushed the sschroevers/additional-reactor-refaster-rules branch from ff27cbe to bf9ea3d Compare January 20, 2024 09:43
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.

In various contexts, suggest more efficient and/or less verbose
constructs.
@rickie rickie force-pushed the sschroevers/additional-reactor-refaster-rules branch from bf9ea3d to 1c930ba Compare January 20, 2024 12:34
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.

Copy link

Quality Gate Passed Quality Gate passed

Kudos, no new issues were introduced!

0 New issues
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

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.

LGTM!

@@ -608,7 +618,7 @@ Flux<S> before(
}

@AfterTemplate
Flux<S> after(Mono<T> mono, Function<? super T, ? extends Iterable<? extends S>> function) {
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 also apply this in the other before template :).

EDIT: It doesn't work, because (correct me if I'm wrong): it doesn't make sense to go from something that is a super type of I to something that is a subtype of I, (or I), right?

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm, it could make sense, but using I instead of ? extends Iterable<? extends S> would impose a greater restriction than necessary or desired: the rule should also work for any Iterable that is not a sub- or super-type of I.

@rickie rickie merged commit 0aa6120 into master Jan 21, 2024
16 checks passed
@rickie rickie deleted the sschroevers/additional-reactor-refaster-rules branch January 21, 2024 12:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

4 participants