GitHub, generate report

https://checkstyle-diff-reports.s3.us-east-2.amazonaws.com/fbdc5a4_2024165552/reports/diff/index.html

GitHub, generate report

We will wait for final report,

but concern me fact that new default behavior is very aggressive, a lot of new violations .
I also find myself liking to do (....) ? ... : ... ; parentheses on condition.

We might not make it default token.

https://checkstyle-diff-reports.s3.us-east-2.amazonaws.com/b34613a_2024195557/reports/diff/index.html

IMO it really is aggressive but isnt the point of the check is to rule out all unnecessary parentheses ? I think it is always subjective to like using parentheses in code for readability even if it , syntactically speaking , does not make a difference if there are parentheses or not

Like with using bitwise operators I think it's more readable to have brackets in this example
((a && b) || c) but this check still rules it as a violation because it should rule out the unnecessary checks IMO unnecessary means in a way that does not affect the code if it's removed
This checks's strictness actually is mentioned here #10946 before and I think it's kind of a tradeoff whether to have strict-unnecessary vs readability especially since it's really subjective but if we go full-unnecessary mode like how the bitwise operators is ruled out then i'd say we have to rule it out for conditional expressions as well

Hey @romani You closed/reopened at the same day so I was wondering WDYT is it too strict / aggressive to add ?

It was inaccurate, pressing on phone. PR is still good. But unfortunately I lost track of it, thanks a lot for pinning. I will meditate for a bit, and will make decision on next 1-2 days.

Please move forward, and choose other issue to contribute.

IMO it really is aggressive but isnt the point of the check is to rule out all unnecessary parentheses ?

yes, it is goal of this Check. BUT there is default behavior and custom behavior.
default behavior is based on some subjective understanding of coding style, in most Checks default mode is not very aggressive.
In custom behavior users can do anything they want.
Check defines target of validation by "tokens".

if users wants to activate this Checks, he will read more on it, to get maximum from it.
If user hesitant, better to not frustrate him on controversial cases, us he might simply disable whole Check. So by making default to be aggressive we potentially decrease usage of Check and can even "kill" it.

We can make example in xdoc pages on how to do this, so users will find it decently quickly.

After looking at may case in diff report.
I see that most people like placing () on expression before ?. It looks like help to parse by brain.

I am voting for not placing this token QUESTION as default to this Check.

@rnveach , @nrmancuso please vote.

One more exercise we can do to prove our subjective idea on default behavior.
In your report we see diff in pmd/spottbugs projects (team that is crazy about code quality) we can send PR to them and see it they accept it.
And people who are less crazy on quality - spring-integration .

if 2 out of 3 accept PR with removal of () it means we can do it as default and Check will be mostly welcomed by engineers.

@romani Pinging as a reminder of what's next ? Should we send the PR to pmd,spotbugs and spring-integration ?

@rdiachenko , please share your opinion on how you like new behavior and if you agree to make it default.

@MohanadKh03 , please send PR to PMD or spotbugs, to test feedback.
you can fix not all cases in their code base, but several, you can share link to report with all to let them see whole picture.

@MohanadKh03, of you have no time for extra PRs , let's trust my experience and just remove tokens from default and put example on how to enable such validation.
With this we will merge this PR quickly.

@romani Yeah my bad I didnt have time for exploring PMD/Spotbugs . Alright ill work on it and the examples should obviously include the token parameter right ?

Yes. What is not enabled by default , is good to show users in examples. Users love examples.

Apologies I have made a mistake while rebasing .. Going to fix rn

Please see commands and videos on how to change PR and rebase https://checkstyle.org/beginning_development.html#Starting_Development

Yes I did I just had problems with mixed old branches into my fork's master which messed things up. Apologies again for the CI spam

Update: Now it is fixed i got all branches together and fixed the first CI failure of not rebasing to master.. Sorry again for the spam

Please always run mvn clean verify on your local before pushing.
It catching most frequent issues. It will speedup collaboration, less pressure on CI

Ah yes I did so .. the first CI problem was due to having been far away from master(job pr-age) that is why there was a mess in rebasing

Ok my bad seems there indeed was a problem while running mvn clean verify
It is kind of weird though the CI and even locally it says that at line 381 there is an additional space 9 spaces and that i should delete one space of it to be 8 even though it already is 8 so somehow it is being modified while running mvn clean verify

After some debugging it seems that the file is being modified while running mvn clean verify only
When I run only this mvn clean org.codehaus.mojo:xml-maven-plugin:1.1.0:check-format it does not have an error and does not modify the file but when using mvn clean verify it seems to modify the unnecessaryparentheses.xml in one of its steps ? Not sure why this happens , Can you please refer me to where I can check for this problem ? @romani

GitHub, generate website

Github, generate report for configs in PR description

https://checkstyle-diff-reports.s3.us-east-2.amazonaws.com/17ed694_2024162640/index.html

https://checkstyle-diff-reports.s3.us-east-2.amazonaws.com/17ed694_2024162640/checks/coding/unnecessaryparentheses.html#Properties

https://checkstyle-diff-reports.s3.us-east-2.amazonaws.com/17ed69479e08dee52e0d7cdfd49eadebfdd5d3a4_2024170408/reports/diff/index.html

@romani Is this xwiki problem related to a certain change I have done ? I tried to debug the script thats failing but cant really get what this really means
[INFO] XWiki Platform - WebSocket ......................... FAILURE [ 0.248 s]

This is Not related failure, we have it from today in all other PRs. I will ignore it.