-
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
Add Refaster template for StepVerifier#create to use Fluent API #18
Conversation
@@ -59,6 +59,10 @@ | |||
return ImmutableSet.of(PublisherProbe.of(Mono.empty()), PublisherProbe.of(Flux.empty())); | |||
} | |||
|
|||
ImmutableSet<StepVerifier.FirstStep<Integer>> testStepVerifierCreate() { |
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.
Wasn't too sure whether it would be better to split the test, such that the test better reflects the two templates. WDYT?
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.
I'd indeed split the test in two. Makes finding the respective test easier.
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 could also combine the templates (though that's slightly... juck). We do try to have one test method for each template, so yes, let's split.
@@ -194,6 +194,32 @@ private ReactorTemplates() {} | |||
} | |||
} | |||
|
|||
/** Use the fluent API style when using {@link StepVerifier#create} for {@link Mono}. */ | |||
static final class StepVerifierCreateMono<T> { |
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.
Had a "hacky" idea, where I combined the two Refaster templates in one template.
So that one before template has Flux<T>
and the other Mono<T>
with the same aftertemplate.
It is somewhat of workaround, but I had to try.
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.
Yep, was pondering the same (see my comment below ;) ), but this might bite us if we ever want to statically validate the "sanity" of a given template. So fine to leave as-is.
...r-prone-contrib/src/main/java/tech/picnic/errorprone/refastertemplates/ReactorTemplates.java
Outdated
Show resolved
Hide resolved
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.
Overall LGTM
I'm trusting you on the last point. 👍
@@ -59,6 +59,10 @@ | |||
return ImmutableSet.of(PublisherProbe.of(Mono.empty()), PublisherProbe.of(Flux.empty())); | |||
} | |||
|
|||
ImmutableSet<StepVerifier.FirstStep<Integer>> testStepVerifierCreate() { |
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.
I'd indeed split the test in two. Makes finding the respective test easier.
55de1e9
to
30c6647
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.
Rebased and added a few tweaks.
Suggested commit message:
Add Refaster rules for `StepVerifier` creation (#18)
@@ -59,6 +59,10 @@ | |||
return ImmutableSet.of(PublisherProbe.of(Mono.empty()), PublisherProbe.of(Flux.empty())); | |||
} | |||
|
|||
ImmutableSet<StepVerifier.FirstStep<Integer>> testStepVerifierCreate() { |
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 could also combine the templates (though that's slightly... juck). We do try to have one test method for each template, so yes, let's split.
@@ -194,6 +194,32 @@ private ReactorTemplates() {} | |||
} | |||
} | |||
|
|||
/** Use the fluent API style when using {@link StepVerifier#create} for {@link Mono}. */ | |||
static final class StepVerifierCreateMono<T> { |
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.
Yep, was pondering the same (see my comment below ;) ), but this might bite us if we ever want to statically validate the "sanity" of a given template. So fine to leave as-is.
No description provided.