Skip to content
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

Remove '//ok' comments from Input files #13213

Open
romani opened this issue Jun 13, 2023 · 65 comments
Open

Remove '//ok' comments from Input files #13213

romani opened this issue Jun 13, 2023 · 65 comments

Comments

@romani
Copy link
Member

romani commented Jun 13, 2023

After hundreds of example PR reviews, I , and not only I found that comment ok is just a noise that makes it hard to read.
This example is small, so you will not fill pain, but, there are more complicated examples and it is not good.
I agree to put comment to explain some not obvious reason, but it will be exception.

For now I treat all //ok comments as stuff to remove, there might be cases to keep them as form of:
// ok, here is not obvious reason
// ok, here is pointing to Check limitation
In short - when it is not obvious.

all suppression <suppress id="UnnecessaryOkComment" for at https://github.com/checkstyle/checkstyle/blob/master/config/checkstyle-input-suppressions.xml should be removed and referenced files updated to not have "// ok" comments

We do need this removals in test inputs.
Same is for Example files, it just makes example full of content that is hard to distinguish without code coloring.

user should see is comment as red flag of attention.
All other lines are without attention, as no violation on them, on all of them.

We use to have such comments only at times when we had no enforcing and it was very manual markers , but it lost its role after we finished Input based tests

Such ok comment become to appear in completely unrelated places, by copy paste, and keep it at right place is not possible( or hard and outside of this project). Better without it .
Location of violation comment is enforced by runtime - never outdated.

Please choose name of module from file:

<!-- until https://github.com/checkstyle/checkstyle/issues/13213 -->
<suppress id="UnnecessaryOkComment"
files="checks[\\/]coding[\\/]equalsavoidnull[\\/]InputEqualsAvoidNullEnhancedInstanceof.java"/>

where name of module is equalsavoidnull and send PR for all files that belong to it in single PR.

final goal is to remove all suppressions of "UnnecessaryOkComment".

Example of update: https://github.com/checkstyle/checkstyle/pull/13609/files attention on removal of suppression
See more examples below for "Merged" Pull Requests.

@romani
Copy link
Member Author

romani commented Jun 13, 2023

@Vyom-Yadav , @Kevin222004 , @rdiachenko , @nrmancuso , please share your opinion on this issue.

@nrmancuso
Copy link
Member

nrmancuso commented Jun 13, 2023

Agree 100%, I think overuse of comments diminishes the concision of examples and inputs.

@Vyom-Yadav
Copy link
Member

Agreed, let's only permit // violation and // ok, because of ........

@Kevin222004
Copy link
Contributor

I also agree to remove // ok.

@rdiachenko
Copy link
Contributor

user should see is comment as red flag of attention.
All other lines are without attention, as no violation on them, on all of them.

+1

@romani
Copy link
Member Author

romani commented Jun 14, 2023

@stoyanK7 , please share your position on this

@stoyanK7
Copy link
Contributor

I also agree with removing // ok

@romani
Copy link
Member Author

romani commented Jun 16, 2023

@rnveach , please share your thoughts

nrmancuso added a commit to nrmancuso/checkstyle that referenced this issue Aug 19, 2023
@romani
Copy link
Member Author

romani commented Aug 20, 2023

@sheetalj2205, we are ready to start removal. Referenced PR is merged.

@github-actions github-actions bot added this to the 10.12.3 milestone Aug 20, 2023
@sheetalj2205
Copy link

@sheetalj2205, we are ready to start removal. Referenced PR is merged.

Okay, I am on it.

@sheetalj2205
Copy link

sheetalj2205 commented Aug 24, 2023

Hello @romani I just want to check before moving forward whether I understood it correctly or not, as we can see in these files InputDefaultComesLastDefaultMethodsInInterface.java and illegaltype/InputIllegalTypeEmptyStringMemberModifiers.java there are multiple //ok comments, we need to remove that and put it only at one place that is after class or interface name and I am supposed to do that for all the files of com.puppycrawl.tools.checkstyle.checks.coding package.

@romani
Copy link
Member Author

romani commented Aug 24, 2023

We need to remove all //ok comments and keep only comments that some text after it like //ok bla bla bla

@sheetalj2205
Copy link

We need to remove all //ok comments and keep only comments that some text after it like //ok bla bla bla

can you please assign this to me

@nrmancuso
Copy link
Member

@sheetalj2205 we do not assign issues except to maintainers, just make a comment like “I am on XXXCheck” and send a PR.

@romani
Copy link
Member Author

romani commented Aug 24, 2023

@sheetalj2205, just make comment "I am xxxxxCheck tests" and send PR for single Check only. After we merge first PR you can send PRs for more, by group. We do not allow huge PRs that change all, such PRs usually never come to merge state.

@nrmancuso
Copy link
Member

I am on MethodParamPadCheck

AadhilMR added a commit to AadhilMR/checkstyle that referenced this issue Mar 30, 2024
AadhilMR added a commit to AadhilMR/checkstyle that referenced this issue Mar 30, 2024
AadhilMR added a commit to AadhilMR/checkstyle that referenced this issue Mar 30, 2024
AadhilMR added a commit to AadhilMR/checkstyle that referenced this issue Mar 30, 2024
AadhilMR added a commit to AadhilMR/checkstyle that referenced this issue Mar 30, 2024
AadhilMR added a commit to AadhilMR/checkstyle that referenced this issue Mar 30, 2024
AadhilMR added a commit to AadhilMR/checkstyle that referenced this issue Mar 30, 2024
Acesif added a commit to Acesif/checkstyle that referenced this issue Apr 1, 2024
@github-actions github-actions bot modified the milestones: 10.15.0, 10.15.1 Apr 2, 2024
Prathamesh-007 pushed a commit to Prathamesh-007/checkstyle that referenced this issue Apr 3, 2024
Prathamesh-007 pushed a commit to Prathamesh-007/checkstyle that referenced this issue Apr 3, 2024
prathm3 added a commit to prathm3/checkstyle that referenced this issue Apr 24, 2024
prathm3 added a commit to prathm3/checkstyle that referenced this issue Apr 25, 2024
prathm3 added a commit to prathm3/checkstyle that referenced this issue Apr 25, 2024
prathm3 added a commit to prathm3/checkstyle that referenced this issue Apr 26, 2024
prathm3 added a commit to prathm3/checkstyle that referenced this issue Apr 29, 2024
prathm3 added a commit to prathm3/checkstyle that referenced this issue Apr 29, 2024
@github-actions github-actions bot modified the milestones: 10.16.0, 10.16.1 Apr 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests