-
-
Notifications
You must be signed in to change notification settings - Fork 3.8k
Issue #17366: FinalParameters should check interface methods and patt… #17368
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
Conversation
6277e43
to
ec49fc7
Compare
Do I have to do anything about the javac17_standard test failing? That's kinda the purpose of noncompilable test sources? |
0e3aca5
to
bf797d8
Compare
We need to send fixes to Orekit project |
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.
Items
.../resources/com/puppycrawl/tools/checkstyle/checks/finalparameters/InputFinalParameters3.java
Outdated
Show resolved
Hide resolved
...puppycrawl/tools/checkstyle/checks/finalparameters/InputFinalParametersPatternVariables.java
Show resolved
Hide resolved
bf797d8
to
a709d2d
Compare
Ok, addressed those 2 points.
The fixes aren't to anything new, it doesn't include the pattern variable def token by default, so the changes are just gonna be actual methods with method bodies on interfaces. Like this, and this. |
I know, but we can not merge on red CI. |
a709d2d
to
a69ad21
Compare
I guess I can also make a boolean option to skip interfaces |
It is not good option. It is easier to send PR to Orekit |
a69ad21
to
e03a36d
Compare
Ok, PR created: https://gitlab.orekit.org/orekit/orekit/-/merge_requests/841 |
0218fd2
to
4fd3319
Compare
Ok, orekit updated. Should be good to go for another review. |
Github, generate report for FinalParameters/all-examples-in-one |
Report for FinalParameters/all-examples-in-one: |
Ok, looks like there's an issue with pattern variable def in for each, I'll check that. EDIT: oh that was removed, and was never released as a non-preview feature. And it wasn't supported before, should it be now? |
After fix please trigger regression diff testing as I did, it will speed up PR acceptance |
This Check is aggressive by its nature. As you see diff is massive. I don't think we should do such property now, but we should put in issue example how to bring behavior to previous state by suppression of violations under interfaces by https://checkstyle.org/filters/suppressionxpathsinglefilter.html#SuppressionXpathSingleFilter so users can simply copy paste config and not be blocked checkstyle upgrade by huge amount of violations. It might need few iterations for them to fix such violations. So suppression would be appreciated. |
ebcd434
to
f395565
Compare
Github, generate report for FinalParameters/all-examples-in-one |
f395565
to
959141f
Compare
Ok, suppression filter created: #17366 (comment) |
Report for FinalParameters/all-examples-in-one: |
Ok, issue with java20 record pattern in foreach is fixed. |
959141f
to
47c83c1
Compare
@@ -165,33 +166,40 @@ public int[] getRequiredTokens() { | |||
|
|||
@Override | |||
public void visitToken(DetailAST ast) { | |||
// don't flag interfaces |
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.
this comment was added 23 years ago - f313f31
not clear why, so it is ok to do interfaces validation now.
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'm not sure when static methods were added to interfaces, I'm pretty sure both default and private methods were added later, like java 8/9, but I thought static methods were always available on interfaces. 🤷
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.
last items:
...ols/checkstyle/checks/finalparameters/InputFinalParametersRecordForLoopPatternVariables.java
Show resolved
Hide resolved
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.
items:
.../puppycrawl/tools/checkstyle/checks/finalparameters/InputFinalParametersInterfaceMethod.java
Show resolved
Hide resolved
report is good. |
47c83c1
to
8b04718
Compare
Ok, changes addressed. |
56c321c
to
88b6911
Compare
Ok, fixed it by updating image. |
88b6911
to
c83d5a3
Compare
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.
Thanks a lot for update.
Ok to merge
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.
Great work, thanks for your contribution!
rebasing on latest master before merge |
…s and pattern variable definitions
Fixes #17366