Prefer flatMapIterable(identity()) over flatMap(i -> FluxfromIterable(i))
#279
Conversation
|
Looking at the build failures. |
Ptijohn
added
new feature
blocked
| flux.flatMap(list -> Flux.fromIterable(list), concurrency), | ||
| flux.flatMap(Flux::fromIterable, concurrency)); |
There was a problem hiding this comment.
Hum, I guess that if concurrency is 1, we want to rewrite that to concatMapIterable to be consistent with rules above 🤔
So maybe I'll add a new entry to FluxConcatMapFromIterable above that rewrites specifically flatMap(..., 1) 🤔
Is there such a thing as priority of rules?
As this actually conflicts with FluxConcatMap...
|
Alright, there's a bit of extra work here than I thought here (see comment above), to make sure to have the proper rewrites in all cases. |
There was a problem hiding this comment.
(The over-arching idea here is that each Refaster rule provides a "minimal" improvement, so that we maximize the amount of code cleaned up, without suffering from a combinatorial explosion of different rules.)
Thanks for this contribution @Ptijohn! This one definitely occurs in our code base 😄
I added a commit. Suggested commit message:
Suggest `Flux#concatMap{,Iterable}` usage in more contexts (#279)
| @AfterTemplate | ||
| @UseImportPolicy(STATIC_IMPORT_ALWAYS) | ||
| Flux<S> after(Flux<? extends Iterable<S>> flux, int concurrency) { | ||
| return flux.flatMapIterable(identity(), concurrency); |
There was a problem hiding this comment.
Here the second argument is the prefetch value; there is no concurrency here. So i.c.w. your suggestion above, I think we need:
- A rewrite for the
flatMap(..., 1)case (which is indeed whatFluxConcatMapalready does, though I see we can addprefetchoverrides). - The
concatMapIterablecheck above :)
There was a problem hiding this comment.
Thanks! That makes sense :)
I would have gone this direction today, but well you beat me to it ;)
| * Prefer {@link Flux#concatMapIterable(Function)} over {@link Flux#concatMap(Function)} with | ||
| * {@link Flux#fromIterable(Iterable)}. | ||
| */ | ||
| static final class FluxConcatMapFromIterable<S> { |
There was a problem hiding this comment.
Our yet-to-be-codified naming conventions would have this template be named ConcatMapIterableIdentity :)
There was a problem hiding this comment.
Yeah I was struggling a bit on the naming convention 😇 Learning as I go!
| * Prefer {@link Flux#concatMapIterable(Function)} over {@link Flux#concatMap(Function)} with | ||
| * {@link Flux#fromIterable(Iterable)}. |
There was a problem hiding this comment.
Let's say something about avoiding inner subscriptions.
| Flux.just(ImmutableList.of("1")).concatMap(list -> Flux.fromIterable(list)), | ||
| Flux.just(ImmutableList.of("1")).concatMap(Flux::fromIterable)); |
There was a problem hiding this comment.
In tests we generally use integers, or the strings "foo", "bar", "baz", etc.
There was a problem hiding this comment.
Ah yeah fair enough :)
Ptijohn
force-pushed
the
bdiederichs/ANS-1289
branch
from
a74fed0 to
651905b
Compare
|
Rebased + resolved conflicts :) |
| * Prefer {@link Flux#concatMapIterable(Function)} over alternatives that require an additional | ||
| * subscription. | ||
| */ | ||
| static final class ConcatMapIterableIdentity<S> { |
There was a problem hiding this comment.
We use S when we need two generics, in this case we can simply use <T>.
There was a problem hiding this comment.
Ah yes :). (Also something to auto-unify on day 🙈.)
Suggested commit message: