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.

Sign up for GitHub

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

Jump to bottom

Issue #11885: Allow SuppressWarningHolder to suppress the violation with NameCheck #11889

Merged
merged 1 commit into from Jul 29, 2022
Merged

Issue #11885: Allow SuppressWarningHolder to suppress the violation with NameCheck #11889

merged 1 commit into from Jul 29, 2022

Conversation

stoyanK7
Copy link
Collaborator

@stoyanK7 stoyanK7 commented Jul 13, 2022 •
edited

Resolves #11885
Allows SuppressWarningHolder to suppress violations with NameCheck i.e @SuppressWarnings("checkstyle:constantnamecheck") or @SuppressWarnings("checkstyle:ConstantNameCheck")

Diff Regression config: https://gist.githubusercontent.com/stoyanK7/73d7efde0efd9c6496ba95f0816ccb86/raw/0e0179ee0b4e193cb5faa8b5fcfa74a6bc1a46f0/config.xml

@stoyanK7 stoyanK7 force-pushed the issue/11885-allow-SuppressWarningHolder-to-suppress-violation-with-NameCheck branch 2 times, most recently from a6cb1c5 to f1f7275 Compare July 13, 2022 15:18
@stoyanK7
Copy link
Collaborator Author

Github, generate report

@github-actions
Copy link
Contributor

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

romani
romani approved these changes Jul 14, 2022
View reviewed changes
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.

Ok to merge

@romani romani requested a review from rnveach July 14, 2022 13:32
@stoyanK7
Copy link
Collaborator Author

I guess documentation is missing still.

Attention: documentation must be updated to mention in words that this is possible, not example is required (as it is rarely used names, in all docs all names are without Check).

Do either of these sound suitable?

Adding check suffix is also accepted.

or

Adding check suffix is optional.
Screenshot from 2022-07-14 18-27-21

@romani
Copy link
Member

romani commented Jul 15, 2022

Adding check suffix is also accepted

Let's use this

@stoyanK7 stoyanK7 force-pushed the issue/11885-allow-SuppressWarningHolder-to-suppress-violation-with-NameCheck branch from f1f7275 to 24e2b28 Compare July 15, 2022 10:19
@romani
Copy link
Member

romani commented Jul 15, 2022

@stoyanK7 , please resolve conflict

@stoyanK7 stoyanK7 force-pushed the issue/11885-allow-SuppressWarningHolder-to-suppress-violation-with-NameCheck branch from 24e2b28 to 28c0708 Compare July 15, 2022 16:31
@stoyanK7
Copy link
Collaborator Author

Done!
Changed from
Screenshot from 2022-07-15 19-25-38
to
Screenshot from 2022-07-15 19-31-10

@stoyanK7
Copy link
Collaborator Author

Github, generate site

@github-actions
Copy link
Contributor

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

rnveach
rnveach reviewed Jul 16, 2022
View reviewed changes
src/main/java/com/puppycrawl/tools/checkstyle/checks/SuppressWarningsHolder.java Outdated
@@ -97,8 +97,8 @@
* </pre>
* <p>
* The general rule is that the argument of the {@code @SuppressWarnings} will be
* matched against class name of the checker in lower case and without {@code Check}
* suffix if present.
* matched against class name of the checker in any letter case. Adding {@code check}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

checker

I am assuming this is suppose to be check or module.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is correct.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@stoyanK7 , please fix this typo "checker" -> "Check".

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done!

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Checks are now passing too!

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rnveach ping

@stoyanK7 stoyanK7 force-pushed the issue/11885-allow-SuppressWarningHolder-to-suppress-violation-with-NameCheck branch from 28c0708 to 8ba68b7 Compare July 16, 2022 14:42
@stoyanK7 stoyanK7 force-pushed the issue/11885-allow-SuppressWarningHolder-to-suppress-violation-with-NameCheck branch from 8ba68b7 to dc1c5a4 Compare July 17, 2022 16:56
rnveach
rnveach reviewed Jul 28, 2022
View reviewed changes
Copy link
Member

@rnveach rnveach left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My only remaining comment is there doesn't seem to be any examples showing "Name" and "NameCheck" and "NaMeChEcK" is supported.

@rnveach rnveach assigned romani and unassigned rnveach Jul 28, 2022
@stoyanK7
Copy link
Collaborator Author

Yeah, I only wrote

matched against class name of the checker in any letter case

Original issue said only to mention it in words, without example. I can add an example if you really want one :)

@rnveach
Copy link
Member

rnveach commented Jul 29, 2022

@romani can clarify if an example is needed

romani
romani approved these changes Jul 29, 2022
View reviewed changes
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.

Example is not required, we already have enough examples.

@romani romani merged commit 419a491 into checkstyle:master Jul 29, 2022
@stoyanK7 stoyanK7 deleted the issue/11885-allow-SuppressWarningHolder-to-suppress-violation-with-NameCheck branch July 29, 2022 06:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rnveach rnveach rnveach left review comments

@romani romani romani approved these changes

@strkkk strkkk strkkk approved these changes

Assignees

@romani romani

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Allow SuppressWarningHolder to suppress the violation with NameCheck
4 participants
@stoyanK7 @romani @rnveach @strkkk