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

Migrate doAfterSuccessOrError to tap #9

Draft
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

Laurens-W
Copy link
Contributor

What's your motivation?

Further improving coverage of 3.5 and 3.6 deprecations as seen here https://projectreactor.io/docs/core/3.4.39/api/deprecated-list.html

Anything in particular you'd like reviewers to focus on?

Test coverage
Setup of the recipe

Anyone you would like to review specifically?

@OlegDokuka
@timtebeek

Checklist

  • I've added unit tests to cover both positive and negative cases
  • I've read and applied the recipe conventions and best practices
  • I've used the IntelliJ IDEA auto-formatter on affected files

@Laurens-W Laurens-W self-assigned this Oct 9, 2024
@Laurens-W Laurens-W added the enhancement New feature or request label Oct 9, 2024
Comment on lines +49 to +54
if (error != null) {
System.out.println("error" + error);
} else {
System.out.println("success" + result);
}
System.out.println("other logs");
Copy link
Contributor

@timtebeek timtebeek Oct 9, 2024

Choose a reason for hiding this comment

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

Great start I think to split out commands in this way; let's add a few more variants like if (error == null), if (null == error), or separate ifs, no else, other statements before/interspersed. We're going for a bit of variety here, such that we don't overfit the recipe implementation to this one test. Then from there we can look across OSS projects that use doAfterSuccessOrError.

Copy link
Contributor Author

@Laurens-W Laurens-W Oct 9, 2024

Choose a reason for hiding this comment

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

I already ran a FindMethods across OSS and found none, but perhaps pattern reactor.core.publisher.Mono doAfterSuccessOrError(..) is too specific?

Will add more test variations!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
Status: In Progress
Development

Successfully merging this pull request may close these issues.

2 participants