Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Error Prone #2054

Closed
vorburger opened this issue Jun 26, 2023 · 5 comments · Fixed by #2176
Closed

Add Error Prone #2054

vorburger opened this issue Jun 26, 2023 · 5 comments · Fixed by #2176
Labels
theme: build An issue or change related to the build system type: enhancement ✨

Comments

@vorburger
Copy link
Contributor

How about adding https://errorprone.info/docs/installation to PicoCLI? Using (https://github.com/tbroyer/gradle-errorprone-plugin.)

It would have found the #2053 problem, and possibly others, and will prevent regressions of them, and future new bugs.

@remkop would a PR for this be a welcome contribution?

@remkop
Copy link
Owner

remkop commented Jun 28, 2023

Hi @vorburger, yes that seems like a good idea!

One note of caution may be with the different Java versions.
When building picocli and creating releases, I use JDK 8.
Additionally, the GitHub Actions-based CI compiles and runs tests using various other JDK versions.
Both use cases would need to be supported.

There is a separate subproject for compiling/running on Java 6/7, we don't need to add ErrorProne to that.

If that is possible, then let's go for it!

@remkop remkop added type: enhancement ✨ theme: build An issue or change related to the build system labels Jun 28, 2023
@remkop remkop added this to the 4.7.5 milestone Jun 28, 2023
@clebertsuconic
Copy link
Contributor

clebertsuconic commented Jul 31, 2023

I have a love / hate relationship with errorprone.

While it gets some issues eventually, it generates a lot of noise that unless you are willing to fix every single warning coming from errorprone those will get ignored as they become noise.

Picocli is not that huge, so it won't add much time to the build, but one issue I have with it in artemis is that it takes a good amount to time to process.

IMO though, picocli is very small. and I appreciate the fact it is small. I don't see much benefit from using errorprone.

@vorburger
Copy link
Contributor Author

IMO though, picocli is very small. and I appreciate the fact it is small.

@clebertsuconic adding Error Prone to the PicoCLI build (only!) will keep the PicoCLI JAR and API as small as it is... 😺

When building picocli and creating releases, I use JDK 8. Additionally, the GitHub Actions-based CI compiles and runs tests using various other JDK versions. Both use cases would need to be supported.

@remkop I see! Because you use a Matrix Build it wouldn't "just work". Given https://errorprone.info/docs/installation stating "Please note that Error Prone must be run on JDK 11 or newer.", the best way maybe to simply have the gradle-errorprone-plugin run only on Java 11+ ... I've never done this with Gradle, but given how imperative Gradle is, and that it can bake Pizzas in addition to building software, presumably there is some way to make it so... does anyone reading this happen to know how to do that? Otherwise I'll look a bit more into it when I have some time.

@vorburger
Copy link
Contributor Author

presumably there is some way to make it so

Asked on https://discuss.gradle.org/t/how-to-skip-disable-plugin-execution-based-on-java-version/47274

Will also update https://stackoverflow.com/a/77668885/421602 (which comes up when searching for this).

@vorburger
Copy link
Contributor Author

WIP on main...vorburger:picocli:errorprone ...

... currently shows 2 errors, 100 warnings - I think at least for an initial version the simplest here is to hide the warnings and fix the 2 errors.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
theme: build An issue or change related to the build system type: enhancement ✨
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants