Skip to content

Issue #17487: Add errorprone.refasterrules #17760

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.

Sign up for GitHub

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

Jump to bottom
Merged
romani merged 1 commit into from
Sep 13, 2025
Merged

Issue #17487: Add errorprone.refasterrules #17760

romani merged 1 commit into from
Sep 13, 2025

Conversation

@Pankraz76
Copy link

@Pankraz76 Pankraz76 commented Sep 8, 2025 •
edited
Loading

Fixes: #16543
Fixes: #17487

tech.picnic.errorprone.refasterrules.StreamRulesRecipes$StreamMapFirstRecipe: Where possible, clarify that a mapping operation will be applied only to a single stream element.

[WARNING] Changes have been made to src/main/java/com/puppycrawl/tools/checkstyle/site/ModuleJavadocParsingUtil.java by:
[WARNING]     tech.picnic.errorprone.refasterrules.StreamRulesRecipes
[WARNING]         tech.picnic.errorprone.refasterrules.StreamRulesRecipes$StreamMapFirstRecipe

@Pankraz76 Pankraz76 force-pushed the prerequisite-UpgradeToJava17 branch from 76cbf97 to 049779c Compare September 8, 2025 10:05
@Pankraz76 Pankraz76 changed the title Issue #17734: Add org.checkstyle.CheckstyleImportLayout as prerequisite for Rewrite:UpgradeToJava17 Issue #17734: Add Checkstyle Sanity Check Sep 8, 2025
@Pankraz76 Pankraz76 force-pushed the prerequisite-UpgradeToJava17 branch 2 times, most recently from e312cc0 to a058bc5 Compare September 8, 2025 13:19
@Pankraz76 Pankraz76 changed the title Issue #17734: Add Checkstyle Sanity Check Issue #17734: Add Checkstyle Sanity Check featuring picnic.errorprone Sep 8, 2025
@Pankraz76 Pankraz76 mentioned this pull request Sep 8, 2025
Merged
@Pankraz76 Pankraz76 changed the title Issue #17734: Add Checkstyle Sanity Check featuring picnic.errorprone ### Issue #17487: Add Checkstyle Sanity Check featuring picnic.errorprone Sep 8, 2025
@Pankraz76 Pankraz76 changed the title ### Issue #17487: Add Checkstyle Sanity Check featuring picnic.errorprone Issue #17487: Add Checkstyle Sanity Check featuring picnic.errorprone Sep 8, 2025
@Pankraz76 Pankraz76 force-pushed the prerequisite-UpgradeToJava17 branch from a058bc5 to 4b755ec Compare September 8, 2025 13:22
@Pankraz76 Pankraz76 mentioned this pull request Sep 8, 2025
Closed
@Pankraz76 Pankraz76 changed the title Issue #17487: Add Checkstyle Sanity Check featuring picnic.errorprone Issue #17487: Add Sanity Check featuring picnic.errorprone Sep 8, 2025
@Pankraz76 Pankraz76 force-pushed the prerequisite-UpgradeToJava17 branch from 4b755ec to 18762c8 Compare September 8, 2025 13:24
@Pankraz76
Copy link
Author

Pankraz76 commented Sep 8, 2025

kindly asking for feedback @Anmol202005 @romani @rdiachenko, thanks.

Pankraz76
Pankraz76 commented Sep 8, 2025
View reviewed changes
@Pankraz76 Pankraz76 force-pushed the prerequisite-UpgradeToJava17 branch 3 times, most recently from 380f124 to ea26f6d Compare September 8, 2025 14:08
@Pankraz76 Pankraz76 marked this pull request as ready for review September 8, 2025 14:09
@Pankraz76 Pankraz76 changed the title Issue #17487: Add Sanity Check featuring picnic.errorprone Issue #17487: Add Sanity Check featuring picnic.errorprone.StreamRulesRecipes Sep 8, 2025
@Pankraz76 Pankraz76 force-pushed the prerequisite-UpgradeToJava17 branch 4 times, most recently from a99c3e5 to fe91f09 Compare September 9, 2025 12:22
@Pankraz76 Pankraz76 mentioned this pull request Sep 9, 2025
Closed
4 tasks
@Pankraz76 Pankraz76 changed the title Issue #17487: Add Sanity Check featuring picnic.errorprone.StreamRulesRecipes Issue #17487: Add CheckstyleSanityCheck featuring CheckstyleRefasterRules & CheckstyleStaticAnalysis Sep 10, 2025
@Pankraz76 Pankraz76 force-pushed the prerequisite-UpgradeToJava17 branch from fe91f09 to 23218b4 Compare September 10, 2025 19:32
@Pankraz76 Pankraz76 mentioned this pull request Sep 10, 2025
Merged
@Pankraz76 Pankraz76 force-pushed the prerequisite-UpgradeToJava17 branch from 23218b4 to 2082079 Compare September 10, 2025 21:43
romani
romani requested changes Sep 11, 2025
View reviewed changes
Copy link
Member

@romani romani left a comment

Choose a reason for hiding this comment

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

Items

@Pankraz76 Pankraz76 force-pushed the prerequisite-UpgradeToJava17 branch from 2082079 to 0312961 Compare September 11, 2025 11:11
@Pankraz76 Pankraz76 force-pushed the prerequisite-UpgradeToJava17 branch from afa5b31 to ecc4a07 Compare September 11, 2025 12:06
@Pankraz76 Pankraz76 requested a review from romani September 11, 2025 13:16
rickie
rickie reviewed Sep 11, 2025
View reviewed changes
Copy link
Contributor

@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.

Small suggestion.

@rickie rickie mentioned this pull request Sep 11, 2025
Closed
@Pankraz76 Pankraz76 changed the title Issue #17487: Add CheckstyleSanityCheck featuring CheckstyleRefasterRules & CheckstyleStaticAnalysis Issue #17487: Add CheckstyleSanityCheck Sep 12, 2025
@Pankraz76 Pankraz76 force-pushed the prerequisite-UpgradeToJava17 branch 2 times, most recently from eacbc3e to c688a4e Compare September 12, 2025 10:09
@Pankraz76
Copy link
Author

Pankraz76 commented Sep 12, 2025

exec. time 10m 29s, going to need increase timeout soon.

romani
romani requested changes Sep 12, 2025
View reviewed changes
Copy link
Member

@romani romani left a comment

Choose a reason for hiding this comment

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

Items

@Pankraz76 Pankraz76 changed the title Issue #17487: Add CheckstyleSanityCheck Issue #17487: Add rewrite support for errorprone.refasterrules Sep 12, 2025
@Pankraz76 Pankraz76 changed the title Issue #17487: Add rewrite support for errorprone.refasterrules Issue #17487: Add errorprone.refasterrules Sep 12, 2025
@Pankraz76 Pankraz76 force-pushed the prerequisite-UpgradeToJava17 branch from c688a4e to 4d102f8 Compare September 12, 2025 18:48
@Pankraz76 Pankraz76 requested review from rickie and romani September 12, 2025 18:48
@Pankraz76 Pankraz76 force-pushed the prerequisite-UpgradeToJava17 branch from 4d102f8 to 3e27e4b Compare September 12, 2025 18:50
Pankraz76
Pankraz76 commented Sep 12, 2025
View reviewed changes
@romani
Copy link
Member

romani commented Sep 12, 2025 •
edited
Loading

Please do not resolve my review item, just reply "done" or any other comment .

I will resolve them as confirmation that is is done as expected

@romani
Copy link
Member

romani commented Sep 12, 2025

Conflict

romani
romani requested changes Sep 12, 2025
View reviewed changes
Copy link
Member

@romani romani left a comment

Choose a reason for hiding this comment

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

Items

pom.xml Outdated
<recipe>org.openrewrite.java.migrate.Java8toJava11</recipe>
<recipe>CheckstyleAutoFix</recipe>
<recipe>OpenRewrite</recipe>
<recipe>Picnic</recipe>
Copy link
Member

@romani romani Sep 12, 2025

Choose a reason for hiding this comment

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

Please put name spaces for all of this 3 recipe groups

Copy link
Author

@Pankraz76 Pankraz76 Sep 13, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

what you mean by grouping? How to do so?

Maybe full qualified/package name?

Suggested change
<recipe>Picnic</recipe>
<recipe>tech.picnic.Picnic</recipe>

please give me suggestion, or c&p instructions.

No need to interpret and get lost.

Copy link
Author

@Pankraz76 Pankraz76 Sep 13, 2025

Choose a reason for hiding this comment

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

boiled down to avoid overhead.

Lets extract afterwards, if needed.

Copy link
Member

@romani romani Sep 13, 2025

Choose a reason for hiding this comment

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

does it make sense to make it tech.picnic.errorprone ?

and org.openrewrite ?

it will be just a name provider of recipie.

Copy link
Member

@romani romani Sep 13, 2025

Choose a reason for hiding this comment

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

@rickie , do you have suggestion on how to organize recipies from different providers ?

Copy link
Contributor

@rickie rickie Sep 15, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

does it make sense to make it tech.picnic.errorprone ?

Yes it does!

@rickie , do you have suggestion on how to organize recipies from different providers ?

Think there are three options:

  • Listing them where they are right now. And have some separating comments to divide them. However that is not necessary per se.
  • Extract the list into 3 separate recipe files per provider. Can be some extra overhead to see which ones are running. Is also maybe overkill for what it achieves.
  • Extract into one large recipe file that has three sections clearly separated by a comment or something explaining that section.

Personally, I'd prefer having one separate recipe file that lists all the recipes.

Copy link
Member

@romani romani Sep 17, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

thanks a lot for feedback.

for now we will go as single block, in future , as execution be above 10 minutes in CI, we will split in multiple (per provider or similar) to keep execution time below 10 minutes.
#17792 (comment)

Copy link
Contributor

@rickie rickie Sep 17, 2025

Choose a reason for hiding this comment

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

I think splitting is not gonna help with bringing the execution time down.

OpenRewrite builds a very large tree (Lossless Semantic Tree) from the source code. Once it did that, it can quickly apply the various changes easily. Splitting will (as far as I know) simply result in multiple builds of 10 minutes.

Copy link
Member

@romani romani Sep 17, 2025

Choose a reason for hiding this comment

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

we will see.

Copy link
Author

@Pankraz76 Pankraz76 Sep 17, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Yes, the tree is the most expensive entity. Splitting isn’t very efficient here, as most of the work would simply be discarded and repeated.

Since we only need about 5-10 extra minutes of runtime, spending another five minutes to build the tree isn’t worthwhile.

If the runtime took many hours, this trade-off would matter less—but for now, a single run seems efficient.

@Pankraz76 Pankraz76 force-pushed the prerequisite-UpgradeToJava17 branch 2 times, most recently from cc894d1 to 638c57d Compare September 13, 2025 10:58
@Pankraz76 Pankraz76 requested a review from romani September 13, 2025 11:01
@romani
Copy link
Member

romani commented Sep 13, 2025

please resolve conflict

@Pankraz76 Pankraz76 force-pushed the prerequisite-UpgradeToJava17 branch from 638c57d to f142294 Compare September 13, 2025 14:30
Pankraz76
Pankraz76 commented Sep 13, 2025
View reviewed changes
@Pankraz76 Pankraz76 force-pushed the prerequisite-UpgradeToJava17 branch from f142294 to 9953ea5 Compare September 13, 2025 14:48
romani
romani approved these changes Sep 13, 2025
View reviewed changes
Copy link
Member

@romani romani left a comment

Choose a reason for hiding this comment

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

Ok, let's keep moving, but advise from @rickie still appreciated

@romani romani merged commit 60b519e into checkstyle:master Sep 13, 2025
120 checks passed
@rickie
Copy link
Contributor

rickie commented Sep 15, 2025

Thanks for tagging me. Missed the notification in the weekend @romani , answered here: #17760 (comment).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@romani romani romani approved these changes

@rickie rickie Awaiting requested review from rickie

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Add rewrite support for errorprone.refasterrules implement IDE agnostic configuration with editorconfig.org

3 participants

@Pankraz76 @romani @rickie