Skip to content

Conversation

Machine-Maker
Copy link
Contributor

Fixes #17366

@Machine-Maker
Copy link
Contributor Author

Do I have to do anything about the javac17_standard test failing? That's kinda the purpose of noncompilable test sources?

@Machine-Maker Machine-Maker force-pushed the issue-17366 branch 2 times, most recently from 0e3aca5 to bf797d8 Compare July 12, 2025 21:08
@romani
Copy link
Member

romani commented Jul 12, 2025

We need to send fixes to Orekit project
https://checkstyle.semaphoreci.com/jobs/c3b3a06d-159d-4508-aed8-40876262544f
To see user reaction on new violations before release to whole world.

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

@Machine-Maker
Copy link
Contributor Author

Ok, addressed those 2 points.

We need to send fixes to Orekit project checkstyle.semaphoreci.com/jobs/c3b3a06d-159d-4508-aed8-40876262544f To see user reaction on new violations before release to whole world.

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.

@romani
Copy link
Member

romani commented Jul 12, 2025

I know, but we can not merge on red CI.
May be they have suppression that we can extend?

@Machine-Maker
Copy link
Contributor Author

I guess I can also make a boolean option to skip interfaces

@romani
Copy link
Member

romani commented Jul 13, 2025

It is not good option.

It is easier to send PR to Orekit

@Machine-Maker
Copy link
Contributor Author

@Machine-Maker Machine-Maker force-pushed the issue-17366 branch 2 times, most recently from 0218fd2 to 4fd3319 Compare July 13, 2025 19:18
@Machine-Maker
Copy link
Contributor Author

Ok, orekit updated. Should be good to go for another review.

@romani
Copy link
Member

romani commented Jul 13, 2025

Github, generate report for FinalParameters/all-examples-in-one

Copy link
Contributor

@Machine-Maker
Copy link
Contributor Author

Machine-Maker commented Jul 13, 2025

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?

@romani
Copy link
Member

romani commented Jul 13, 2025

After fix please trigger regression diff testing as I did, it will speed up PR acceptance

@romani
Copy link
Member

romani commented Jul 13, 2025

This Check is aggressive by its nature.
But fact that we did not have complains before on missed violations might imply that world not not firm on where to put violations and where to skip.
This kind of reference to your idea to put property to skip in interfaces.

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.

@Machine-Maker Machine-Maker force-pushed the issue-17366 branch 2 times, most recently from ebcd434 to f395565 Compare July 13, 2025 22:48
@Machine-Maker
Copy link
Contributor Author

Github, generate report for FinalParameters/all-examples-in-one

@Machine-Maker
Copy link
Contributor Author

Ok, suppression filter created: #17366 (comment)

Copy link
Contributor

@Machine-Maker
Copy link
Contributor Author

Machine-Maker commented Jul 13, 2025

Ok, issue with java20 record pattern in foreach is fixed.

@Machine-Maker Machine-Maker requested a review from romani July 16, 2025 01:15
@@ -165,33 +166,40 @@ public int[] getRequiredTokens() {

@Override
public void visitToken(DetailAST ast) {
// don't flag interfaces
Copy link
Member

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.

Copy link
Contributor Author

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

@Machine-Maker Machine-Maker requested a review from romani July 18, 2025 01:12
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.

last items:

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:

@romani
Copy link
Member

romani commented Jul 20, 2025

report is good.
Please do cosmetic changes requested above and we will proceed further.

@Machine-Maker
Copy link
Contributor Author

Ok, changes addressed.

@Machine-Maker Machine-Maker requested a review from romani July 21, 2025 18:32
@Machine-Maker Machine-Maker force-pushed the issue-17366 branch 3 times, most recently from 56c321c to 88b6911 Compare July 21, 2025 18:54
@Machine-Maker
Copy link
Contributor Author

Machine-Maker commented Jul 21, 2025

I'm unsure why the javac21 test is failing, that code seems to compile fine for me when I try it.

Ok, fixed it by updating image.

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.

Thanks a lot for update.
Ok to merge

Copy link
Member

@nrmancuso nrmancuso left a 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!

@nrmancuso nrmancuso assigned romani and unassigned nrmancuso Jul 22, 2025
@nrmancuso
Copy link
Member

rebasing on latest master before merge

@romani romani merged commit 8c8cfd4 into checkstyle:master Jul 22, 2025
118 checks passed
@Machine-Maker Machine-Maker deleted the issue-17366 branch July 23, 2025 04:46
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.

FinalParameters - missing several tokens to check
3 participants