Issue #17697: Add check for box comments#18986
Conversation
3bb84ad to
7e11c64
Compare
|
github, generate site |
romani
left a comment
There was a problem hiding this comment.
please run diff report on project: guava, comuda (they use google style)
items
|
|
||
| void myFunc4() { | ||
| // ######### | ||
| // violation above 'Comment should not be enclosed in a box.' |
There was a problem hiding this comment.
as we cannot detect box reliably, we should change wording of violation to something like (wording can be improved)
Comment should not be repetitive usage of '*' , '=', '#' that usually used in box like comments.
There was a problem hiding this comment.
Did try this violation message, but such errors were there..
examples\resources\com\puppycrawl\tools\checkstyle\checks\todocomment\Example3.java:8: Line is longer than 85 characters (found 88). [LineLength]
[ERROR] [checkstyle] [ERROR] C:\Users\Smita Prajapati\checkstyle\src\xdocs-examples\resources\com\puppycrawl\tools\checkstyle\checks\todocomment\Example3.java:17: Discouraged usage of escaped special regex symbols.
Have changed it to Comment uses box-like repetitive character pattern. to better fit the use-case
e054f0a to
9b3b087
Compare
|
GitHub, generate report |
|
Report generation failed on phase "make_report", |
|
GitHub, generate report |
|
BoxComments check on Google Style projects: |
|
Report generation failed on phase "make_report", |
|
@romani please review |
|
@romani ping |
|
@romani plz take a look |
|
CI is red |
4247698 to
766d786
Compare
eaf3dd1 to
9d6fdbe
Compare
|
Done |
| "checks/regexp/regexpmultiline/Example5", | ||
| "checks/sizes/lambdabodylength/Example2", | ||
| "checks/sizes/linelength/Example6", | ||
| "checks/todocomment/Example3", |
There was a problem hiding this comment.
I have some incomplete idea.
What if we have two sections for examples. One section will be to show all properties in action and all code can be same. And another section to random useful use cases that we just share and code for it can be any, do not need to same as others.
There was a problem hiding this comment.
This way:
// xdoc section -- start
// Section 1: property showcase (consistent structure)
class ExampleN {
// same fields, same methods across all examples
}
// xdoc section -- end
// xdoc section (use-case) -- start
// Section 2: real-world use case
class UseCaseN {
// whatever makes sense for this specific config
}
// xdoc section (use-case) -- end
we can have this structure as I feel it would make examples to be more readable and complex unification wont be required much. but need to discuss for below :
will both sections appear in the generated xml?
will violation test run against both?
There was a problem hiding this comment.
I actually through on section in xdoc/html, that reuse the same macroses, but fully excluded from validation on same non comment AST..
https://checkstyle.org/checks/misc/todocomment.html#Examples is for same non comment AST .
And under it new section. "Use cases".
With examples of any description and config and java codes. In our case it will be https://checkstyle-diff-reports.s3.us-east-2.amazonaws.com/7e11c64_20260222171827/checks/misc/todocomment.html#Example3-config
There was a problem hiding this comment.
If I am not wrong is this what you are suggesting:
Section 1 — "Examples" (existing, same as now):
Same xdoc markers, same macros
All examples MUST have identical non-comment AST
XdocsExamplesAstConsistencyTest validates this section
Example: https://checkstyle.org/checks/misc/todocomment.html#Examples
Section 2 — "Use Cases" (new):
New section in the HTML page, below Examples
Uses same macros/rendering pipeline , just a new heading
BUT fully excluded from XdocsExamplesAstConsistencyTest
Any code structure, any config, no consistency requirement
Example: https://checkstyle-diff-reports.s3.us-east-2.amazonaws.com/7e11c64_20260222171827/checks/misc/todocomment.html#Example3-config
So from implementation perspective:
No new xdoc markers needed in Java files
New section heading in the .xml doc file
XdocsExamplesAstConsistencyTest gets a new set like USE_CASE_EXAMPLES that are excluded from validation
Same rendering, just organized differently on the page
There was a problem hiding this comment.
Yes, you got it.
Just new title of section, below it examples from real life.
If you agree and no problems you see, we can create separate issue and do it later.
This PR we will merge as is for now
There was a problem hiding this comment.
Yes, we can create a separate issue for this.
My only concern is whether we should first finish the long-running suppression issue #18435 and then introduce this new "Use Cases" section approach, or handle both in parallel since this would also reduce the need for many suppressions and AST consistency workarounds.
There was a problem hiding this comment.
Please create new issue, as agreement point.
We will finish suppression list first, and we will keep in mind that some examples just need to be moved to "Use cases", so should not be forced on same AST
There was a problem hiding this comment.
For example this example will be in that group for sure.
So as we proceed on examples we will move what we noticed to to new group of suppression under comment " until ((new issue))"
505d952 to
cce6879
Compare
|
GitHub, generate website |
| <span class="wrapper inline"> | ||
| <img src="images/ok_green.png" alt="" /> | ||
| </span> | ||
| BoxComments |
There was a problem hiding this comment.
Please put here search link as others have
There was a problem hiding this comment.
I tried adding the search link but it causes XdocsPagesTest to fail with "too many config links" for the 4.8.6.1 row. The test only allows one config link per module entry
[ERROR] Failures:
[ERROR] XdocsPagesTest.testAllStyleRules:1962->validateStyleModules:2094->validateChapterWiseTesting:2198 google_style.xml rule '4.8.6.1 Block comment style' module 'BoxComments' has too many config links
[INFO]
[ERROR] Tests run: 6000, Failures: 1, Errors: 0, Skipped: 2
[IN.
There was a problem hiding this comment.
Ok, please revert back and create new issue on this nuance, to let us improve separately.
861d727 to
7a1ee12
Compare
#17697
Adds detection for comments enclosed in boxes drawn with special characters, providing partial coverage for Google Java Style Guide §4.8.6: "Comments are not enclosed in boxes drawn with asterisks or other characters."
Implementation
Uses the existing
TodoCommentcheck with a custom configuration:Check ID:
BoxCommentsRegex Pattern:
^[ \t]*([*=#-])\1{8,}[ \t]*$Detection:
Lines with 9+ consecutive repeated characters (*, =, #)Changes
Configuration
Modified:
src/main/resources/google_checks.xmlAdded
TodoCommentmodule with id="BoxComments"Added suppressions for test files with intentional box-style comments
Documentation
Modified: src/xdocs/google_style.xml
Added row for §
4.8.6.3showing partial coverageModified:
src/xdocs/checks/misc/todocomment.xmlAdded
Example 3demonstrating box comment detectionCreated:
src/xdocs/resources/com/puppycrawl/tools/checkstyle/checks/todocomment/Example3.javaExample configuration and test cases
Tests
Created:
src/it/java/com/google/checkstyle/test/chapter4formatting/rule4863boxcomments/BoxCommentsTest.javaIntegration test for Google style
Created:
src/it/resources/com/google/checkstyle/test/chapter4formatting/rule4863boxcomments/InputBoxComments.javaTest input with various box comment patterns
Modified:
src/test/java/com/puppycrawl/tools/checkstyle/checks/TodoCommentCheckExamplesTest.javaAdded
testExample3()for box comment examplesDiff Regression config: https://raw.githubusercontent.com/smita1078/checkstyle/BoxComments/src/main/resources/google_checks.xml
Diff Regression projects: https://gist.githubusercontent.com/smita1078/f26521cfc945a2c966396eabd09bc1e8/raw/8d1de4376f538633f1d9f394494ba7315b40d81f/projects-google-style.properties
Report label: BoxComments check on Google Style projects