Issue #17487: Add errorprone.refasterrules#17760
Conversation
76cbf97 to
049779c
Compare
org.checkstyle.CheckstyleImportLayout as prerequisite for Rewrite:UpgradeToJava17Checkstyle Sanity Check
e312cc0 to
a058bc5
Compare
Checkstyle Sanity CheckCheckstyle Sanity Check featuring picnic.errorprone
Checkstyle Sanity Check featuring picnic.errorproneCheckstyle Sanity Check featuring picnic.errorprone
Checkstyle Sanity Check featuring picnic.errorproneCheckstyle Sanity Check featuring picnic.errorprone
a058bc5 to
4b755ec
Compare
Checkstyle Sanity Check featuring picnic.errorproneSanity Check featuring picnic.errorprone
4b755ec to
18762c8
Compare
|
kindly asking for feedback @Anmol202005 @romani @rdiachenko, thanks. |
380f124 to
ea26f6d
Compare
Sanity Check featuring picnic.errorproneSanity Check featuring picnic.errorprone.StreamRulesRecipes
a99c3e5 to
fe91f09
Compare
Sanity Check featuring picnic.errorprone.StreamRulesRecipesCheckstyleSanityCheck featuring CheckstyleRefasterRules & CheckstyleStaticAnalysis
fe91f09 to
23218b4
Compare
23218b4 to
2082079
Compare
2082079 to
0312961
Compare
afa5b31 to
ecc4a07
Compare
CheckstyleSanityCheck featuring CheckstyleRefasterRules & CheckstyleStaticAnalysisCheckstyleSanityCheck
eacbc3e to
c688a4e
Compare
|
exec. time 10m 29s, going to need increase timeout soon. |
CheckstyleSanityCheckrewrite support for errorprone.refasterrules
rewrite support for errorprone.refasterruleserrorprone.refasterrules
c688a4e to
4d102f8
Compare
4d102f8 to
3e27e4b
Compare
|
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 |
|
Conflict |
| <recipe>org.openrewrite.java.migrate.Java8toJava11</recipe> | ||
| <recipe>CheckstyleAutoFix</recipe> | ||
| <recipe>OpenRewrite</recipe> | ||
| <recipe>Picnic</recipe> |
There was a problem hiding this comment.
Please put name spaces for all of this 3 recipe groups
There was a problem hiding this comment.
what you mean by grouping? How to do so?
Maybe full qualified/package name?
| <recipe>Picnic</recipe> | |
| <recipe>tech.picnic.Picnic</recipe> |
please give me suggestion, or c&p instructions.
No need to interpret and get lost.
There was a problem hiding this comment.
boiled down to avoid overhead.
Lets extract afterwards, if needed.
There was a problem hiding this comment.
does it make sense to make it tech.picnic.errorprone ?
and org.openrewrite ?
it will be just a name provider of recipie.
There was a problem hiding this comment.
@rickie , do you have suggestion on how to organize recipies from different providers ?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
cc894d1 to
638c57d
Compare
|
please resolve conflict |
638c57d to
f142294
Compare
f142294 to
9953ea5
Compare
|
Thanks for tagging me. Missed the notification in the weekend @romani , answered here: #17760 (comment). |
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.