style: Add errorprone warnings. #592
Conversation
a796401 to
585e117
Compare
|
I've adapted my original PR to use profiles to only apply ErrorProne to JDK 9 and beyond. I don't think the GraalVM test failure is my doing, as it appears to be missing some configuration file I've not touched. I believe this is ready for review. |
fe8ca1b to
3077aa2
Compare
|
I believe this is again ready for review. |
|
ping |
|
Oh, so close 😮 |
kolea2
left a comment
There was a problem hiding this comment.
LGTM with one remaining question
pom.xml
Outdated
| <artifactId>maven-compiler-plugin</artifactId> | ||
| <version>3.8.0</version> | ||
| <configuration> | ||
| <source>8</source> |
There was a problem hiding this comment.
just curious if we need this since this profile is for jdk 9+ only?
There was a problem hiding this comment.
Ah, possibly not. I think I can just use
<configuration>
<release>9</release>
</configuration>
Lemme try that.
There was a problem hiding this comment.
Latest version uses that and seems to be fine with it.
There was a problem hiding this comment.
I think you can simply remove it? I was looking at https://github.com/googleapis/java-pubsublite/blob/adb22207fb3cba10de95a5e6399456d362408dd7/pom.xml#L166-L199
There was a problem hiding this comment.
I was under the understanding if you didn't have either <source> or <target> or <release>, the default behavior was that it was treated as Java 6 (i.e., 1.6), but perhaps that was in earlier releases of Java? Anyway, removing it also works.
1aae18b to
9057499
Compare
Related to googleapis#590.
This introduces use of the errorprone plugin as described at https://errorprone.info/docs/installation#maven. The warnings can then be addressed,
@SuppressWarnings(…)ed, or if desired, have entire classes of warnings turned off. Then once the codebase is clean, the warnings can be treated as errors. This will reduce the drift between github and the monorepo, which generally tries to enforce errorprone cleanliness.Related to #590.