API: Fix retainAll and removeAll in CharSequenceSet #7133
Conversation
There was a problem hiding this comment.
@zhongyujiang great catch, this looks good to me. Can you add a test to TestCharSequenceSet? cc @rdblue @jackye1995
|
@amogh-jahagirdar Thanks for your comment, when adding tests I found that currently In the new commit, the passed in |
zhongyujiang
changed the title
|
The failed task is iceberg-spark:iceberg-spark-3.2_2.12:checkstyleTest, not relevant to this. |
|
|
||
| Assert.assertTrue("Set should be changed", set.retainAll(ImmutableList.of("456", "789", 123))); | ||
| Assert.assertTrue("Should not retain element \"123\"", set.size() == 1); | ||
| Assert.assertTrue("Should retain element \"456\"", set.contains("456")); |
There was a problem hiding this comment.
let's rewrite L46+L47 to Assertions.assertThat(set).hasSize(1).contains("456"). Same for all the places below.
Also let's try and avoid assertTrue / assertFalse as much as possible as it doesn't provide enough context if a check ever fails (See #7131 for some background)
There was a problem hiding this comment.
Done, also changed L45 and other boolean assertions, this uses overridingErrorMessage to provide error message and thefore increases the number of lines, but allows us not to mix Assert and Assertions. Do you think it's OK?
There was a problem hiding this comment.
yep that looks good, thanks
| public boolean removeAll(Collection<?> objects) { | ||
| if (objects != null) { | ||
| return Iterables.removeAll(wrapperSet, objects); | ||
| return objects.stream().filter(this::remove).count() != 0; |
There was a problem hiding this comment.
why not do the same as in L142ff?
There was a problem hiding this comment.
removeAll iterates the objects and checks whether these elements exist in the removeFrom set, in this case we don't need to wrap all elements at once, use this#remove can take advantage of the wrapper in the ThreadLocal wrappers.
| return false; | ||
| } | ||
|
|
||
| @Override | ||
| @SuppressWarnings("CollectionUndefinedEquality") |
There was a problem hiding this comment.
is this one necessary? I don't see errorprone complaining about it
There was a problem hiding this comment.
It complained in the previous CI.
Found this while browsing the code, and checked that this method does not seem to be used in the project at present, luckily.