Skip to content

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

Merged
merged 1 commit into from
Jul 1, 2025

Conversation

@mohitsatr
Copy link
Member Author

Github, generate report

Copy link
Contributor

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

@mohitsatr mohitsatr force-pushed the whitespace-after-comment branch from 80c1640 to 75956b0 Compare June 12, 2025 06:06
@mohitsatr mohitsatr force-pushed the whitespace-after-comment branch from 75956b0 to eb3f1e1 Compare June 13, 2025 04:46
@mohitsatr
Copy link
Member Author

we always knew how to handle comments

<module name="MatchXpath">
<property name="id" value="singleLineCommentStartWithSpace"/>
<property name="query"
value="//SINGLE_LINE_COMMENT[./COMMENT_CONTENT[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'))]]"/>

Comment on lines 66 to 71
<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'))]]"/>
Copy link
Member

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 ?

Copy link
Member

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.

Copy link
Member

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

Copy link
Member Author

@mohitsatr mohitsatr Jun 14, 2025

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.

Copy link
Member

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

Copy link
Member Author

@mohitsatr mohitsatr Jun 19, 2025

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.

Copy link
Member Author

@mohitsatr mohitsatr Jun 19, 2025

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 ?

Copy link
Member

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 🫠 )

Copy link
Member

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.

Copy link
Member Author

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

@mohitsatr
Copy link
Member Author

Github, generate report

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:

@@ -1860,6 +1860,7 @@ public void testAllStyleRules() throws Exception {
styleChecks.remove("SuppressWarningsFilter");
styleChecks.remove("SuppressWarningsHolder");
styleChecks.remove("SuppressWithNearbyTextFilter");
styleChecks.remove("MatchXpath");
Copy link
Member

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.

Copy link
Member Author

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 ?

Copy link
Member

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.

Copy link
Member

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

done.

Copy link
Member

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

Copy link
Member Author

Choose a reason for hiding this comment

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

removed.

Copy link
Contributor

@mohitsatr mohitsatr force-pushed the whitespace-after-comment branch 2 times, most recently from c2b50c0 to 033ae0c Compare June 16, 2025 04:26
@Zopsss
Copy link
Member

Zopsss commented Jun 18, 2025

Please make formatter happy @mohitsatr

@Zopsss
Copy link
Member

Zopsss commented Jun 18, 2025

The PR is almost ready, I just need confirmation at: #17206 (comment) and also add MatchXpath usage to coverage table as @romani suggested.

@mohitsatr mohitsatr force-pushed the whitespace-after-comment branch from 033ae0c to 4ca2d1e Compare June 19, 2025 10:10
@mohitsatr
Copy link
Member Author

@Zopsss formatter can't be happy. It is violating on inputs.

@Zopsss
Copy link
Member

Zopsss commented Jun 19, 2025

@mohitsatr Exclude it and create InputFormatted file for it to make formatter happy

@mohitsatr mohitsatr force-pushed the whitespace-after-comment branch 2 times, most recently from 1044e06 to 0c1981b Compare June 19, 2025 12:36
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:

@mohitsatr mohitsatr force-pushed the whitespace-after-comment branch from 0c1981b to b21a9fb Compare June 27, 2025 06:11
@Zopsss
Copy link
Member

Zopsss commented Jun 27, 2025

@mohitsatr any updates on this?

@mohitsatr
Copy link
Member Author

this is ready to merge. we were waiting for #17264 to be merged.

@romani
Copy link
Member

romani commented Jun 27, 2025

PR is merged , please proceed.

@mohitsatr
Copy link
Member Author

@romani This pr is finished and is ready to ready to merge

@romani
Copy link
Member

romani commented Jun 30, 2025

Last clarification #17206 (comment)

@mohitsatr mohitsatr force-pushed the whitespace-after-comment branch from b21a9fb to 22216b0 Compare July 1, 2025 06:15
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.

Thanks a lot

@romani romani merged commit 396ac5e into checkstyle:master Jul 1, 2025
118 of 119 checks passed
@mohitsatr mohitsatr deleted the whitespace-after-comment branch July 2, 2025 03:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Google-style: Improper enforcement of horizontal whitespace for double slash //
3 participants