consider new PatchLocation mode `IN_PLACE_INCLUSIVE`
- Dominant language
- Java
- Stars
- 7.2k
- Forks
- 820
- Avg merge
- 5h 9m
- Merged PRs (30d)
- 50
Description
Hello folks,
Thanks for the great tool and the opportunity to contribute to JUnit and other projects, as seen in:
- https://github.com/junit-team/junit-framework/pull/5006
- https://github.com/gradle/gradle/pull/35278
- https://github.com/checkstyle/checkstyle/pull/17892
All of this originated from and revolves around this tool.
While adapting it repeatedly, I've noticed the `IN_PLACE` behavior can be inconsistent. Sometimes it works without the required counterpart `XepPatchChecks` (which is another story), as encountered in:
- https://github.com/gradle/gradle/pull/35107
I have experienced that `IN_PLACE` is exclusive, meaning it only runs the checks passed through the other variable. Please consider a new hybrid mode, perhaps `IN_PLACE_INCLUSIVE`, which would apply the rewrite approach—meaning it applies all suggested fixes. Some checks will have fixes, others won't. For simple stuff like ordering, this is about avoiding annoyance and improving overall performance.
``` gradle
if (!getenv().containsKey('CI') && getenv('IN_PLACE')?.toBoolean()) {
errorproneArgs.addAll(
'-XepPatchLocation:IN_PLACE',
'-XepPatchChecks:RedundantStringConversion'
)
}
```
While patching is considered experimental, others consider it good enough, and this idea would support that argument.
Origination from:
- https://github.com/junit-team/junit-framework/pull/5006#issuecomment-3384697369
---
## Explanation
Sorry for the confusion, and thanks for considering.
This is just a new **error-prone check**, originating from **Picnic**, and it behaves the same as other rules in all builds. If the check finds an issue, it will **fail the build** and **show a suggested fix**, just like the current ones.
We can’t use the **optional patching mode (`IN_PLACE`)** because it overrides the rule settings when activated. While this could work locally, it would require significant overhead—like maintaining an explicit whitelist to keep the ruleset in sync. Otherwise, a locally green build might fail in CI, as some checks aren't included, creating a rule delta between local and CI builds. This is not ideal and would lead to manual intervention, which patching is meant to avoid (similar to what's done with rewrite).
This means only the specified checks would run locally, leading to a **successful local build** but a **failed CI build**, since CI does not patch and still enforces all rules for quality control.
To have patching, we would either need to:
- Adapt **Error Prone** to support this as a new feature, or
- Create a whitelist approach similar to **Spotless**.
Both options are **unrealistic**, so we should move forward with the current setup and leave the patching topic aside for now—to stop staring and start finishing. This can be revisited later, along with considerations like `StaticImport`.
Currently, **Spotless** uses a **fully configured ruleset**, which checks only the configured rules instead of following the convention. While this allows **auto-fix capability**, it’s not ideal for maintaining consistency with conventions.
Given that the required changes here are minimal, keeping our current setup is the better approach.
**Related:**
- https://github.com/diffplug/spotless/pull/2664/files#r2416889516
- https://github.com/gradle/gradle/pull/35107
- https://github.com/apache/kafka/pull/20688
- https://github.com/diffplug/spotless/pull/2664
Contributor guide
Assessment
This issue has not been assessed yet.