-
-
Notifications
You must be signed in to change notification settings - Fork 3.8k
Issue #17193: Enforce whitespace after the //
of the comments
#17206
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
Conversation
Github, generate report |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Items
...e/test/chapter4formatting/rule462horizontalwhitespace/InputWhitespaceAfterDoubleSlashes.java
Show resolved
Hide resolved
80c1640
to
75956b0
Compare
...e/test/chapter4formatting/rule462horizontalwhitespace/InputWhitespaceAfterDoubleSlashes.java
Show resolved
Hide resolved
75956b0
to
eb3f1e1
Compare
we always knew how to handle comments checkstyle/config/checkstyle-checks.xml Lines 472 to 478 in 296a360
|
...chapter4formatting/rule462horizontalwhitespace/InputWhitespaceAfterDoubleSlashesCorrect.java
Show resolved
Hide resolved
src/main/resources/google_checks.xml
Outdated
<property name="query" | ||
value="//SINGLE_LINE_COMMENT[./COMMENT_CONTENT[not(starts-with(@text, ' ')) | ||
and not(starts-with(@text, '/')) | ||
and not(@text = '\n') and not(ends-with(@text, '//\n')) | ||
and not(@text = '\r') and not(ends-with(@text, '//\r')) | ||
and not(@text = '\r\n') and not(ends-with(@text, '//\r\n'))]]"/> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need to check for \n
, \r
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
to avoid false positive on empty comment, i think.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mohitsatr can you investigate it? If it is missing such empty comments cases then please add them
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for /n'
, it is empty comments//
. they are already in inputs.
I’m not entirely sure about /r
, but since it’s already in our codebase, I assume it has been thoroughly tested and is there for a reason.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you try to remove \r
from checkstyle-checks.xml and run mvn clean verify
locally? If build fails then we will be able to find test cases for it, add them to google Inputs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I first removed and not(@text = '\r')
and ran the built, then I also removed and not(@text = '\r\n')
and built. no errors on both builts.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wait! I use Linux, so it'd build successfully anyway right ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd try it. I use window ( I'm noob 🫠 )
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tested it and build doesn't fail.
Config:
<module name="MatchXpath">
<property name="id" value="singleLineCommentStartWithSpace"/>
<property name="query"
value="//SINGLE_LINE_COMMENT[./COMMENT_CONTENT[not(starts-with(@text, ' '))
and not(starts-with(@text, '/'))
and not(@text = '\n') and not(ends-with(@text, '//\n'))]]"/>
<message key="matchxpath.match" value="''//'' must be followed by a whitespace."/>
</module>
tried changing both configs: google_checks.xml and checkstyle-checks.xml and build was still passing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For now we can change it in google-config. I will send the changes after #17264
Github, generate report |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
items:
...chapter4formatting/rule462horizontalwhitespace/InputWhitespaceAfterDoubleSlashesCorrect.java
Show resolved
Hide resolved
@@ -1860,6 +1860,7 @@ public void testAllStyleRules() throws Exception { | |||
styleChecks.remove("SuppressWarningsFilter"); | |||
styleChecks.remove("SuppressWarningsHolder"); | |||
styleChecks.remove("SuppressWithNearbyTextFilter"); | |||
styleChecks.remove("MatchXpath"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should document usage of it, it is validation module that should be in coverage table.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you provide an example for how to do this ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
He's suggesting to add MatchXpath
to the coverage, in the rule for which it is used and maybe also explain how it is used.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It should be referenced in table of coverage on a line that requires it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mohitsatr, I still see this removal in PR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removed.
c2b50c0
to
033ae0c
Compare
Please make formatter happy @mohitsatr |
The PR is almost ready, I just need confirmation at: #17206 (comment) and also add MatchXpath usage to coverage table as @romani suggested. |
033ae0c
to
4ca2d1e
Compare
@Zopsss formatter can't be happy. It is violating on inputs. |
@mohitsatr Exclude it and create InputFormatted file for it to make formatter happy |
1044e06
to
0c1981b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
items:
...m/google/checkstyle/test/chapter4formatting/rule43onestatement/InputOneStatementPerLine.java
Outdated
Show resolved
Hide resolved
0c1981b
to
b21a9fb
Compare
@mohitsatr any updates on this? |
this is ready to merge. we were waiting for #17264 to be merged. |
PR is merged , please proceed. |
@romani This pr is finished and is ready to ready to merge |
Last clarification #17206 (comment) |
b21a9fb
to
22216b0
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot
fixes #17193
Diff Regression config: https://gist.githubusercontent.com/mohitsatr/60d8725143067033c0ee8c8b8c6902c0/raw/5602ddde688ca91a52de786a01aca70d74f912dd/google_checks_codeblock.xml
Diff Regression patch config: https://gist.githubusercontent.com/mohitsatr/95cef1a3c86402e641628e7b31984a46/raw/4d9ef3dc3298059f26599742a8dec6ccf549eb30/whitespaceafterdoubleslashs.xml
Diff Regression projects: https://raw.githubusercontent.com/checkstyle/test-configs/refs/heads/main/JavadocTagContinuationIndentation/all-examples-in-one/list-of-projects.properties