Skip to content

Conversation

Aziz-755
Copy link
Contributor

fixes #17167

@Aziz-755
Copy link
Contributor Author

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

Copy link
Contributor

@romani
Copy link
Member

romani commented Jun 15, 2025

@0utplay,
Can you help us validate that this fix actually covers all your problems.

To build jar from this PR, please read #17135 (comment)

Thanks s lot in advance

@Aziz-755
Copy link
Contributor Author

The fix is not done, I still need to do some work. I'll do it ASAP

@0utplay
Copy link

0utplay commented Jun 15, 2025

Hi, just give me another ping and I will test it :)

@Aziz-755 Aziz-755 force-pushed the fpWithIndentationCheckWhenLambdaAndChildOnTheSameLine branch from 7aef136 to 02c5b3c Compare June 15, 2025 22:30
@Aziz-755
Copy link
Contributor Author

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

1 similar comment
@Aziz-755
Copy link
Contributor Author

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

Copy link
Contributor

@Aziz-755
Copy link
Contributor Author

Fix is ready for testing.
@0utplay could you please validate that the fix covers all your problems. Thanks!

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.

Minor

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.

Non functional tems

@romani
Copy link
Member

romani commented Jun 16, 2025

@Aziz-755 , can you check behavior on trinodb
#17163 (comment)
Looks like we have access to their sources, and it is them reporting this issue initially.

We can add their codebase to our CI, no leak problems happening again, and no special reports required to trigger

@0utplay
Copy link

0utplay commented Jun 16, 2025

Fix is ready for testing. @0utplay could you please validate that the fix covers all your problems. Thanks!

Hi, I just tried all of our previous failures. These are now fixed. I could not validate that no new issues arose as we are using the gradle checkstyle plugin.

@Aziz-755
Copy link
Contributor Author

Aziz-755 commented Jun 16, 2025

@Aziz-755 , can you check behavior on trinodb #17163 (comment) Looks like we have access to their sources, and it is them reporting this issue initially.

We can add their codebase to our CI, no leak problems happening again, and no special reports required to trigger

I have checked the behavior on the reported file and the false positives are gone.

If testing checkstyle on the whole project is needed could you please share a guide how can I run checkstyle on a project ? ( I am only familiar with a single file 😅 )

@Aziz-755 Aziz-755 force-pushed the fpWithIndentationCheckWhenLambdaAndChildOnTheSameLine branch from 02c5b3c to c66310a Compare June 16, 2025 16:26
@Aziz-755 Aziz-755 requested a review from romani June 16, 2025 20:18
@romani
Copy link
Member

romani commented Jun 17, 2025

Example for other projects
57962fa

Version override looks like possible at
https://github.com/trinodb/trino/blob/b4b2d9a3d285efca71298acea8f181d775b964bd/pom.xml#L190

@Aziz-755
Copy link
Contributor Author

I tested the changes on the TrinoDB project. I ran Checkstyle 10.25.0 (before the changes) and Checkstyle 10.25.1-SNAPSHOT-all (after the changes) using the following two commands:

D:\Working on Checkstyle>java -jar checkstyle-10.25.1-SNAPSHOT-all.jar -c config.xml trino\ > outputUsing-checkstyle-10.25.1-SNAPSHOT-all

D:\Working on Checkstyle>java -jar checkstyle-10.25.0-all.jar -c config.xml trino\ > outputUsing-checkstyle-10.25.0-all

outputUsing-checkstyle-10.25.0-all.txt
outputUsing-checkstyle-10.25.1-SNAPSHOT-all.txt

You can find the output files attached. All false positives related to this issue have been removed. I also noticed the presence of other violations related to switch statements, and in my opinion, they are valid.

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 !!!!!

@romani romani merged commit d65a112 into checkstyle:master Jun 18, 2025
118 of 119 checks passed
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.

checkstyle expects different indentation for switch cases.
3 participants