Introduce OptionalIdentity Refaster template
#245
Conversation
|
Wouldn't this also be the case for: optional.stream().findAny() |
| * Don't use the redundant chain of {@link Optional#stream()} followed by either {@link | ||
| * Stream#findFirst()} or {@link Stream#findAny()}. | ||
| */ | ||
| abstract static class OptionalSkipStreamFindFirst<T> { |
There was a problem hiding this comment.
@rickie interesting case w.r.t. automated naming: we wouldn't be able to use Optional here. I'll go with OptionalIdentity for now.
...-prone-contrib/src/main/java/tech/picnic/errorprone/refastertemplates/OptionalTemplates.java
Outdated
Show resolved
Hide resolved
| static final class OptionalIdentity<T> { | ||
| @BeforeTemplate | ||
| Optional<T> before(Optional<T> optional) { | ||
| return Refaster.anyOf(optional.stream().findFirst(), optional.stream().findAny()); |
There was a problem hiding this comment.
Perhaps we could also add stream().max() and stream().min()?
There was a problem hiding this comment.
Oh, good one! Sure; wanna add a proposal? 😄
There was a problem hiding this comment.
I'm having a small issue: The test keeps failing because the Input file has the import for java.util.Comparator.reverseOrder, while the Output file doesn't (the output doesn't use any comparator because it doesn't have the max/min function calls). It keeps suggesting to add this (unused) import:
[ERROR] Failures:
[ERROR] RefasterTemplatesTest.validateTemplateCollection:81 value of:
maybeFormat(...)
diff (-expected +actual):
@@ -1,6 +1,7 @@
package tech.picnic.errorprone.refastertemplates;
import static com.google.common.collect.Streams.stream;
+import static java.util.Comparator.reverseOrder;
import com.google.common.collect.ImmutableSet;
import com.google.common.collect.Streams;
I can of course try to fix it by using something that doesn't require an import as the comparator, however it feels strange that it would fail on the imports? Shouldn't the import get removed from the input when the refaster template is applied in the error prone compilation?
There was a problem hiding this comment.
In general it's hard to know whether an import can be removed, because other code may still depend on it (and rules are applied independently). So in practice we instead rely on Google Java Format to drop unused imports after the fact. This does impose a challenge for the tests, as you have noticed. For this purpose we have that funny elidedTypesAndStaticImports method at the top of some of those test classes. But your String::compareTo usage is a nice workaround 👍.
(This does raise another point: we should have a rule that favours naturalOrder() over T::compareTo 😄 But that's for another day/PR.)
There was a problem hiding this comment.
Nice, thanks for the explanation! 😄
|
Anything (else) from you @rickie or @oxkitsune ? 😊 |
|
Yes will review later today :) |
There was a problem hiding this comment.
Nice @svavahb thanks for the nice contribution! 🚀
The sticker is coming your way tomorrow 😉.
Suggested commit message LGTM. Tried thinking about an alternative though, as we sometimes tend to use a different way of formulating commit messages for Refaster templates. Wouldn't be as nice though in this situation.
|
For some reason I cannot add a label 😂 . Can one of you add the label |
|
Works on my machine ;) |
rickie
changed the title
OptionalIdentity Refaster template
As suggested here.
Adds a Refaster template to prefer
optionaloveroptional.stream().findFirst().This is redundant, as
Optional#stream()makes a stream with one element, meaning thefindFirst()will just return the original optional again.(P.s. this is my first contribution, hope I did everything right 😊 And FYI the stickers are absolutely working, I'm 80% doing this for a cute lil ghost on my laptop!!! 👻)