Introduce Comparators{Min,Max} Refaster templates
#270
Conversation
|
Nice! Thanks for opening a PR 🚀 ! This case is a tricky one, make sure to add the |
|
Thanks @rickie for explanation! |
Stephan202
force-pushed
the
nadirbelarouci/binary-operator-min-by
branch
from
b367315
to
d3b766c
Compare
| */ | ||
| static final class BinaryOperatorMinByNaturalOrder<T extends Comparable<? super T>> { | ||
| @BeforeTemplate | ||
| BiFunction<T, T, T> before() { |
There was a problem hiding this comment.
To be more precise:
| BiFunction<T, T, T> before() { | |
| BinaryOperator<T> before() { |
|
|
||
| /** | ||
| * Prefer {@link Comparators#min(Comparable, Comparable)} over {@link | ||
| * BinaryOperator#minBy(Comparator)}for natural ordering. |
There was a problem hiding this comment.
| * BinaryOperator#minBy(Comparator)}for natural ordering. | |
| * BinaryOperator#minBy(Comparator)} for natural ordering. |
But perhaps we can make the text a bit more specific; will propose something.
| * Prefer {@link Comparators#min(Comparable, Comparable)} over {@link | ||
| * BinaryOperator#minBy(Comparator)}for natural ordering. | ||
| */ | ||
| static final class BinaryOperatorMinByNaturalOrder<T extends Comparable<? super T>> { |
There was a problem hiding this comment.
We generally name templates after the @AfterTemplate code. This is a bit of a special case, but the current (not yet officially implemented) algorithm would yield ComparatorsMin.
(The (old) templates further up don't follow this naming rule, and once we change that there'll be a nice clash. But we'll fix that then. I suspect eventually we'll end up with ComparatorsMinAsBinaryOperator, by suffixing the target type for method references.)
| import tech.picnic.errorprone.refaster.test.RefasterTemplateTestCase; | ||
|
|
||
| final class ComparatorTemplatesTest implements RefasterTemplateTestCase { | ||
| @Override | ||
| public ImmutableSet<?> elidedTypesAndStaticImports() { | ||
| return ImmutableSet.of( | ||
| Arrays.class, Collections.class, ImmutableList.class, ImmutableSet.class, identity()); | ||
| Arrays.class, | ||
| BinaryOperator.class, |
There was a problem hiding this comment.
If we use BinaryOperator in the return types, then we don't need this :)
| @@ -10,13 +10,20 @@ | |||
| import java.util.Arrays; | |||
| import java.util.Collections; | |||
| import java.util.Comparator; | |||
| import java.util.function.BiFunction; | |||
| import java.util.function.BinaryOperator; | |||
| import tech.picnic.errorprone.refaster.test.RefasterTemplateTestCase; | |||
|
|
|||
| final class ComparatorTemplatesTest implements RefasterTemplateTestCase { | |||
There was a problem hiding this comment.
I notice that half of the templates are about Comparators rather than Comparator. Maybe something to split one day. 🤷
There was a problem hiding this comment.
Changes LGTM, the suggested commit message as well.
Thanks for opening a PR @nadirbelarouci ! 🚀 🚀
rickie
changed the title
Comparators{Min,Max} Refaster templates
Added 2 rules for
BinaryOperator#minByandBinaryOperator#maxBywhen they're called with anaturalOrdercomparator