Skip to content

Conversation

Pankraz76
Copy link
Contributor

@Pankraz76 Pankraz76 commented Oct 6, 2025

@Pankraz76 Pankraz76 force-pushed the pr-error-prone-SanityCheck branch from 6d13e79 to da00d3a Compare October 6, 2025 14:09
@Pankraz76 Pankraz76 marked this pull request as ready for review October 6, 2025 14:11
@Pankraz76 Pankraz76 force-pushed the pr-error-prone-SanityCheck branch from da00d3a to 75726d1 Compare October 6, 2025 18:29
@Pankraz76 Pankraz76 changed the title Add error-prone.picnic.tech Add error-prone.picnic.tech featuring StaticImport Oct 6, 2025
@Pankraz76 Pankraz76 marked this pull request as draft October 6, 2025 18:58
@Pankraz76 Pankraz76 force-pushed the pr-error-prone-SanityCheck branch from 75726d1 to 7433670 Compare October 7, 2025 09:47
@Pankraz76 Pankraz76 marked this pull request as ready for review October 7, 2025 09:58
@Pankraz76
Copy link
Contributor Author

java.lang.RuntimeException: java.net.SocketTimeoutException: Connect timed out

@Pankraz76
Copy link
Contributor Author

This seems like a good addition to Rewrite, as in some aspects it is superior and has its unique benefits—like every tool has. Of course, they cover the same topic, like MissingOverrideAnnotation, so I'm wondering why we see this changed again. Maybe it's some bug or config issue; it seems to have a great potential when the right tools make the equation round up. It's faster than Rewrite, even though the configuration is not straightforward opt-in like Rewrite. We will work this out and learn on the job while extending the tool.

It's behaving just like Rewrite—failing the build and suggesting changes. The opt-in is quite unique and will not be communicated like in Rewrite, but the changes should be small, like the changesets.

Do you like to see this going on? @nedtwigg Thanks.

Copy link

@rickie rickie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I could be missing some context or another issue / PR in this repository, so please let me know if I miss something.

Not sure about the preference of the maintainers, but we might benefit from a slightly different approach. Now we are enabling a lot of checks in one PR. That makes the PR slightly harder to review. We could benefit from an approach outlined in this comment in JUnit repository.

If we use that approach, reviewing a PR is a lot easier, as all changes relate to one check. In this case there are quite a few that make changes, which makes it harder.

WDYT about that approach?

@Pankraz76 Pankraz76 force-pushed the pr-error-prone-SanityCheck branch from a7f79d2 to 0e2141a Compare October 9, 2025 13:51
@Pankraz76 Pankraz76 changed the title Add error-prone.picnic.tech featuring StaticImport Add error-prone.picnic.tech featuring ConstantNaming Oct 9, 2025
// 'StaticImport',
)
errorproneArgs.add('-XepOpt:Refaster:NamePattern=^(?!.*Rules\\$).*')
if (!getenv().containsKey('CI') && getenv('IN_PLACE')?.toBoolean()) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

activating IN_PLACE allowing removal of allErrorsAsWarnings resulting in local passing, on CI, without patching of course, positive failing build.

How to automate prone without having this glitch? @rickie Patching only cares for the XepPatchChecks like the documentation tells. XepPatchLocation Only works in paid, despite having it seen in gradle not being the case its a very random plugin behaviour sometimes.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only works in paid, despite having it seen in gradle not being the case its a very random plugin behaviour sometimes.

I don't fully understand this comment, can you elaborate?

I agree the usage of the XepPatchChecks and location is a bit finicky and requires some effort to get right. You work on a lot of PRs which is cool to see. But right now I don't have time to dive into all of them and find the exact cause. Will try to also look tomorrow, but can't make promises right now.

I once created a demo project (it's in Maven though) but perhaps you can look into that, and based on that try to configure it for one of the projects you're working on: https://github.com/rickie/error-prone-demo.

@Pankraz76 Pankraz76 force-pushed the pr-error-prone-SanityCheck branch from 0e2141a to 5d58113 Compare October 9, 2025 14:23

tasks.withType(JavaCompile).configureEach {
options.errorprone {
allErrorsAsWarnings = true // ModuleHelper.java:37: warning: Unsafe is internal proprietary API and may be removed in a future release
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

somehow this does not suppress/avoid:

FAILURE: Build failed with an exception.

* What went wrong:
Execution failed for task ':lib:compileJava'.
> Compilation failed; see the compiler output below.
  /Users/vincent.potucek/IdeaProjects/spotless/lib/src/main/java/com/diffplug/spotless/java/ModuleHelper.java:37: warning: Unsafe is internal proprietary API and may be removed in a future release
  import sun.misc.Unsafe;
                 ^
  4 errors
  100 warnings

BUILD FAILED in 10s

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

allErrorsAsWarnings this flag is an Error Prone flag (docs). It doesn't mean that all the errors from the build are being lowered in severity, it's about the checks from Error Prone that are configured with severity ERROR are lowered to WARNING.

In this case there is something else - unrelated to this flag - that is causing the error about the sun.misc.Unsafe. I'm not sure from the top of my head what this error is or why we get it here, but make sure to not misinterpret this flag.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

junit mentions something similar but the fix does not help. Can not handle this issue.

@Pankraz76 Pankraz76 force-pushed the pr-error-prone-SanityCheck branch 2 times, most recently from c8be894 to 1aa9fee Compare October 9, 2025 17:20
@Pankraz76 Pankraz76 changed the title Add error-prone.picnic.tech featuring ConstantNaming Add error-prone.picnic.tech Oct 9, 2025
@Pankraz76 Pankraz76 requested a review from rickie October 9, 2025 17:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants