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 #11655: allow suppressing Google style warnings with annotations and comments #11656

Merged
merged 2 commits into from Jul 8, 2022
Merged

Issue #11655: allow suppressing Google style warnings with annotations and comments #11656

merged 2 commits into from Jul 8, 2022

Conversation

ddcprg
Copy link
Contributor

@ddcprg ddcprg commented May 18, 2022

Fixes #11655

ddcprg
ddcprg commented May 19, 2022
View reviewed changes
src/main/resources/google_checks.xml Outdated
@@ -106,8 +116,7 @@
<property name="tokens"
value="COMMA, SEMI, TYPECAST, LITERAL_IF, LITERAL_ELSE, LITERAL_RETURN,
LITERAL_WHILE, LITERAL_DO, LITERAL_FOR, LITERAL_FINALLY, DO_WHILE, ELLIPSIS,
LITERAL_SWITCH, LITERAL_SYNCHRONIZED, LITERAL_TRY, LITERAL_CATCH, LAMBDA,
LITERAL_YIELD"/>
LITERAL_SWITCH, LITERAL_SYNCHRONIZED, LITERAL_TRY, LITERAL_CATCH, LAMBDA"/>
Copy link
Contributor Author

@ddcprg ddcprg May 19, 2022 •
edited

Choose a reason for hiding this comment

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

LITERAL_YIELD would raise an error in my env which reads this token is not recognised

com.puppycrawl.tools.checkstyle.api.CheckstyleException: cannot initialize module TreeWalker - Token "LITERAL_YIELD" was not found in Acceptable tokens list in check com.puppycrawl.tools.checkstyle.checks.whitespace.WhitespaceAfterCheck
	at com.puppycrawl.tools.checkstyle.Checker.setupChild(Checker.java:476)
	at com.puppycrawl.tools.checkstyle.api.AutomaticBean.configure(AutomaticBean.java:201)
	at com.puppycrawl.tools.checkstyle.Main.runCheckstyle(Main.java:403)
	at com.puppycrawl.tools.checkstyle.Main.runCli(Main.java:330)
	at com.puppycrawl.tools.checkstyle.Main.execute(Main.java:189)
	at com.puppycrawl.tools.checkstyle.Main.main(Main.java:126)
Caused by: com.puppycrawl.tools.checkstyle.api.CheckstyleException: Token "LITERAL_YIELD" was not found in Acceptable tokens list in check com.puppycrawl.tools.checkstyle.checks.whitespace.WhitespaceAfterCheck
	at com.puppycrawl.tools.checkstyle.TreeWalker.registerCheck(TreeWalker.java:223)
	at com.puppycrawl.tools.checkstyle.TreeWalker.setupChild(TreeWalker.java:133)
	at com.puppycrawl.tools.checkstyle.api.AutomaticBean.configure(AutomaticBean.java:201)
	at com.puppycrawl.tools.checkstyle.Checker.setupChild(Checker.java:471)
	... 5 more
Checkstyle ends with 1 errors.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looks like this change has broken the unit tests. I didn't look into why it would fail. I'll push it until I find out

@ddcprg ddcprg mentioned this pull request May 19, 2022
Closed
@nrmancuso
Copy link
Member

nrmancuso commented May 20, 2022 •
edited

@ddcprg issue is approved, please see #11655 (comment) and make CI happy

@nrmancuso nrmancuso self-assigned this May 21, 2022
@nrmancuso nrmancuso self-requested a review May 21, 2022 12:37
@romani
Copy link
Member

romani commented Jun 6, 2022

@ddcprg, ping

@ddcprg
Copy link
Contributor Author

ddcprg commented Jun 6, 2022

@romani I have time to look at this later today

@ddcprg ddcprg force-pushed the issue_11655 branch 6 times, most recently from c5151fd to 3ba2e78 Compare June 6, 2022 15:52
@ddcprg
Copy link
Contributor Author

ddcprg commented Jun 6, 2022

@romani @nick-mancuso please feel free to review any time now

nrmancuso
nrmancuso requested changes Jun 8, 2022
View reviewed changes
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.

Items:

src/main/resources/google_checks.xml Outdated Show resolved Hide resolved
src/main/resources/google_checks.xml Outdated Show resolved Hide resolved
@ddcprg ddcprg force-pushed the issue_11655 branch 3 times, most recently from f009c5b to 4c6c10d Compare June 9, 2022 08:14
@ddcprg
Copy link
Contributor Author

ddcprg commented Jun 9, 2022

@nick-mancuso changes made, please feel free to review again any time

@nrmancuso
Copy link
Member

nrmancuso commented Jun 9, 2022 •
edited

Github, generate site

Link to relevant documentation: https://checkstyle-diff-reports.s3.us-east-2.amazonaws.com/4c6c10d_2022143701/google_style.html#Google_Suppressions

@github-actions
Copy link
Contributor

github-actions bot commented Jun 9, 2022

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

nrmancuso
nrmancuso requested changes Jun 9, 2022
View reviewed changes
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.

We do not have a testing structure in place to test config with suppressions for google style (not that I can find, anyway). Please show CLI output with your new config file and working example with suppressions in place. We can use parts of your example to address the below item:

Item:

src/xdocs/google_style.xml Outdated Show resolved Hide resolved
@ddcprg
Copy link
Contributor Author

ddcprg commented Jun 9, 2022

Github, generate site

Link to relevant documentation: https://checkstyle-diff-reports.s3.us-east-2.amazonaws.com/4c6c10d_2022143701/google_style.html#Google_Suppressions

@nick-mancuso the suppressions section doesn't look very clean with the new details, I can make easier to read if you agreee

romani
romani requested changes Jun 10, 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.

Items

src/main/resources/google_checks.xml Outdated Show resolved Hide resolved
romani
romani requested changes Jun 10, 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.

Items

src/main/resources/google_checks.xml Show resolved Hide resolved
src/main/resources/google_checks.xml Outdated Show resolved Hide resolved
@romani
Copy link
Member

romani commented Jun 26, 2022

@ddcprg, ping

@ddcprg ddcprg force-pushed the issue_11655 branch 2 times, most recently from f68a96d to 1d44145 Compare June 29, 2022 07:17
@ddcprg
Copy link
Contributor Author

ddcprg commented Jun 29, 2022

@romani I can't find the link to the generated site, would you mind telling me where to get it from?

@Vyom-Yadav
Copy link
Member

Github, generate site

@github-actions
Copy link
Contributor

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

@Vyom-Yadav
Copy link
Member

@romani I can't find the link to the generated site, would you mind telling me where to get it from?

To generate it locally https://github.com/checkstyle/checkstyle/wiki/How-to-run-certain-phases-and-validations#how-to-generate-website-only

@ddcprg
Copy link
Contributor Author

ddcprg commented Jun 29, 2022

thank you @Vyom-Yadav @romani @nick-mancuso let me know if the docs look good enough https://checkstyle-diff-reports.s3.us-east-2.amazonaws.com/1d44145_2022082908/google_style.html

romani
romani requested changes Jul 4, 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.

Items

src/xdocs/google_style.xml Outdated Show resolved Hide resolved
romani
romani requested changes Jul 4, 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.

Items

src/xdocs/google_style.xml Outdated Show resolved Hide resolved
src/xdocs/google_style.xml Outdated Show resolved Hide resolved
src/xdocs/google_style.xml Outdated Show resolved Hide resolved
src/xdocs/google_style.xml Outdated Show resolved Hide resolved
@ddcprg
Copy link
Contributor Author

ddcprg commented Jul 6, 2022

Github, generate site

@github-actions
Copy link
Contributor

github-actions bot commented Jul 6, 2022

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

@ddcprg
Copy link
Contributor Author

ddcprg commented Jul 6, 2022

@romani I've made the changes you have requested https://checkstyle-diff-reports.s3.us-east-2.amazonaws.com/cf2a79c_2022063340/google_style.html

romani
romani approved these changes Jul 6, 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

@ddcprg
Copy link
Contributor Author

ddcprg commented Jul 6, 2022

@romani I still can't merge the PR, if you can please press the button

@romani
Copy link
Member

romani commented Jul 6, 2022 •
edited

Only maintainers can merge.
You need to pass review. PR is assigned to next reviewer.

We removed holder from documentation as it is very technical details. It same mention about TreeWalker when you mention about Check. If user activate Check it has to place in config TreeWalker. Same with annotation based filter, holder must be placed in config in same time.

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

@ddcprg thanks a lot for making this happen!

@nrmancuso nrmancuso merged commit a0df0aa into checkstyle:master Jul 8, 2022
@ddcprg ddcprg deleted the issue_11655 branch July 9, 2022 11:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@romani romani romani approved these changes

@nrmancuso nrmancuso nrmancuso approved these changes

Assignees

@nrmancuso nrmancuso

Labels
None yet
Projects
None yet
Milestone
No milestone
4 participants
@ddcprg @nrmancuso @romani @Vyom-Yadav