Prefer com.palantir.common:streams MoreIterables.partition over Iterables.partition#68
Prefer com.palantir.common:streams MoreIterables.partition over Iterables.partition#68schlosna wants to merge 3 commits into
Conversation
Generate changelog in
|
✅ Successfully generated changelog entry!Need to regenerate?Simply interact with the changelog bot comment again to regenerate these entries. 📋Changelog Preview💡 Improvements
|
| .build(); | ||
| return fix(tree, state, "com.google.common.collect.Lists"); | ||
| } | ||
| return fix(tree, state, "com.palantir.common.collect.IterableUtils"); |
There was a problem hiding this comment.
Now that atlasdb is closed-source, I'm not sure it's a good thing to put fixes using internal classes in our public error-prone packages, as this makes it less reusable for others.
Do you think we could put IterableUtils in some of our OSS packages instead? (if needed, maybe having a collection-utils repo/library could be useful?)
There was a problem hiding this comment.
Looking at landing this in streams' MoreIterables palantir/streams#653
There was a problem hiding this comment.
updated to use MoreIterables.partition in streams 2.6.0+
1840493 to
51d570e
Compare
aldexis
left a comment
There was a problem hiding this comment.
Lgtm - just one nit and one question. I'll schedule a reminder to come back to this in a week to check progress of the streams 2.6.0 rollout
| .build(); | ||
| return fix(tree, state, "com.google.common.collect.Lists"); | ||
| } | ||
| return fix(tree, state, "com.palantir.common.streams.MoreIterables"); |
There was a problem hiding this comment.
Out of curiosity, is there a way to check whether that class is on the classpath? Otherwise, let's wait a bit before merging this such that streams 2.6.0 is upgraded in more places (I'll track to come back to this in a week) - see https://pl.ntr/2zk
There was a problem hiding this comment.
@schlosna I haven't forgotten about this by the way, and have been checking every few days on the rollout progress for streams 2.6.0. It seems like we still have ~30 repos that are somehow still on 2.5.0, so I'm still a bit hesitant to merge something that will break baseline-error-prone upgrades due to failing compilation, but the number continues to go down over time. I'll be ooto for the next 10 days, so I'll check back in after I'm back
| "import static com.google.common.collect.Iterables.partition;", | ||
| "class Test {", | ||
| " void test(Iterable<String> items) {", | ||
| " // BUG: Diagnostic contains: Prefer com.palantir.common.streams.MoreIterables.partition", | ||
| " partition(items, 10);", | ||
| " }", |
There was a problem hiding this comment.
nit: At least for new ones (but ideally for this entire file), let's use code blocks, to make it easier to read
| "import static com.google.common.collect.Iterables.partition;", | |
| "class Test {", | |
| " void test(Iterable<String> items) {", | |
| " // BUG: Diagnostic contains: Prefer com.palantir.common.streams.MoreIterables.partition", | |
| " partition(items, 10);", | |
| " }", | |
| // language=Java | |
| """ | |
| import static com.google.common.collect.Iterables.partition; | |
| class Test { | |
| void test(Iterable<String> items) { | |
| // BUG: Diagnostic contains: Prefer com.palantir.common.streams.MoreIterables.partition | |
| partition(items, 10); | |
| } | |
| """ |
There was a problem hiding this comment.
will update this in a bit. may create a separate 🤖 PR to migrate all the old tests
Before this PR
Uses of Guava's
Iterables.partitionpreallocates an array of partition size, which can introduce excessive allocations in cases where the input iterable is smaller than the partition size.After this PR
==COMMIT_MSG==
Prefer use of atlasdb-commons IterableUtils.partition over Iterables.partition
The
com.palantir.common:streamsMoreIterables.partitionimplementation avoids this and provides several optimizations when the input iterable is anImmutableCollectionto return sublists.==COMMIT_MSG==
Possible downsides?