Skip to content

Conversation

Pankraz76
Copy link

@Pankraz76 Pankraz76 commented Sep 29, 2025


I hereby agree to the terms of the JUnit Contributor License Agreement.


Definition of Done

Links:

@Pankraz76 Pankraz76 changed the title add refaster-runner Add error-prone.picnic.tech Oct 3, 2025
@Pankraz76 Pankraz76 marked this pull request as ready for review October 3, 2025 21:45
@Pankraz76 Pankraz76 changed the title Add error-prone.picnic.tech Add error-prone.picnic.tech covering S1488: Local variables should not be declared and then immediately returned or thrown Oct 3, 2025
@Pankraz76 Pankraz76 force-pushed the extend-errorprone branch 2 times, most recently from cb987d5 to c36f592 Compare October 4, 2025 15:02
Copy link
Author

@Pankraz76 Pankraz76 left a comment

Choose a reason for hiding this comment

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

2ct.

@Pankraz76 Pankraz76 requested a review from marcphilipp October 4, 2025 15:08
@Pankraz76 Pankraz76 force-pushed the extend-errorprone branch 2 times, most recently from 8132ffc to 3337a15 Compare October 4, 2025 16:14
@Pankraz76 Pankraz76 changed the title Add error-prone.picnic.tech covering S1488: Local variables should not be declared and then immediately returned or thrown Add error-prone.picnic.tech Oct 4, 2025
Copy link
Author

@Pankraz76 Pankraz76 left a comment

Choose a reason for hiding this comment

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

thanks for the effort spend.

Copy link
Author

@Pankraz76 Pankraz76 left a comment

Choose a reason for hiding this comment

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

ready to go.

@Pankraz76 Pankraz76 requested a review from marcphilipp October 5, 2025 10:38
@Pankraz76 Pankraz76 marked this pull request as draft October 8, 2025 10:49
@Pankraz76 Pankraz76 force-pushed the extend-errorprone branch 2 times, most recently from 2e35ad5 to de9c855 Compare October 8, 2025 13:18
@Pankraz76 Pankraz76 marked this pull request as ready for review October 8, 2025 13:18
@Pankraz76 Pankraz76 changed the title Add error-prone.picnic.tech Add error-prone.picnic.tech featuring RedundantStringConversion Oct 8, 2025
@Pankraz76
Copy link
Author

using the in_place mode only works with whitelisting which is is not the desired case. It would still fail on ci demanding fixes but thats not a good behavior.

Prone demands the manual fixes, not being an rewrite equivlent therefore a new mode might be needed.

@marcphilipp
Copy link
Member

@Pankraz76 Could you please summarize the state of this PR? Is it ready to be reviewed? What do the changes mean for local builds and on CI?

@Pankraz76
Copy link
Author

Pankraz76 commented Oct 9, 2025

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 the other rules for all builds.
If the check finds an issue, it will fail the build and show a suggested fix, just like its currently the case with others.

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 — such as maintaining an explicit whitelist to keep the ruleset in sync.
Otherwise, a locally green build might still fail in CI, as some checks aren’t included, imposing a rule delta between local and CI builds, which is not the ideal path.
This would lead to manual intervention, which patching is meant to avoid (similar to what’s done with rewrite).

That means only the specified checks would run — leading to a successful local build but a failed CI build, since CI does not patch and still enforces all rules for quality control.

In order 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 in terms of maintaining consistency with conventions.

Given that the required changes here are minimal, keeping our current setup is the better approach.

related:

Copy link
Author

@Pankraz76 Pankraz76 left a comment

Choose a reason for hiding this comment

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

now its small enough to be considerable. Thanks.

Copy link
Member

@marcphilipp marcphilipp left a comment

Choose a reason for hiding this comment

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

Thanks for the summary, that helped! 👍

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.

3 participants