-
Notifications
You must be signed in to change notification settings - Fork 486
Add error-prone.picnic.tech
#2664
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
base: main
Are you sure you want to change the base?
Conversation
6d13e79
to
da00d3a
Compare
lib/src/main/java/com/diffplug/spotless/biome/BiomeSettings.java
Outdated
Show resolved
Hide resolved
da00d3a
to
75726d1
Compare
error-prone.picnic.tech
error-prone.picnic.tech
featuring StaticImport
75726d1
to
7433670
Compare
|
d511b46
to
a7f79d2
Compare
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 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. |
There was a problem hiding this 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?
a7f79d2
to
0e2141a
Compare
error-prone.picnic.tech
featuring StaticImport
error-prone.picnic.tech
featuring ConstantNaming
// 'StaticImport', | ||
) | ||
errorproneArgs.add('-XepOpt:Refaster:NamePattern=^(?!.*Rules\\$).*') | ||
if (!getenv().containsKey('CI') && getenv('IN_PLACE')?.toBoolean()) { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
0e2141a
to
5d58113
Compare
gradle/error-prone.gradle
Outdated
|
||
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 |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
c8be894
to
1aa9fee
Compare
error-prone.picnic.tech
featuring ConstantNaming
error-prone.picnic.tech
1aa9fee
to
6a7470c
Compare
relates to:
error-prone.picnic.tech
featuringRedundantStringConversion
junit-team/junit-framework#5006RecipeNullabilityBestPractices
#2663