Issue #6723: new check: LeadingAsteriskAlignCheck

[Zopsss]

Issue: #6723

Introduces a new Module called LeadingAsteriskAlignCheck

Initially the check was for both types of comments, block comments & Javadoc comments and this PR was going to cover up the left over task of @bossever's PR.

But later on we decided to keep this check only for Javadoc comments, here is the conversion link, so I had to almost re-write the whole check implementation. But I want to thanks @bossever for the initial work, it really helped me to implement the check based on new requirements 😊✌️

---

Diff Regression projects: https://gist.githubusercontent.com/Zopsss/22adadb570e4deb95296917244c580b3/raw/29ea756eada8915637b76678db4f46048c198808/projects-to-test-on-for-github-action.properties
New module config: https://gist.githubusercontent.com/Zopsss/69dabf457713431bb900b6144dd7d3ef/raw/f90c10844d6c8645647b2806c6f6ddab910a6ece/JavadocLeadingAsteriskAlign.xml

Contrib repo PR: checkstyle/contribution#848

Fixing Leading Asterisks Alignment in Checkstyle repo: checkstyle/contribution#848
Fixing Leading Asterisks Alignment in Sevntu Repo: sevntu-checkstyle/sevntu.checkstyle#1035

[Zopsss]

I've moved the check from indentation folder to it's individual folder as it was throwing an error something like LeadingAsteriskAlignCheck is not in 'misc' module.

And also I was facing error while running mvn clean verify, here is the error log:

[ERROR]   MetadataGeneratorUtilTest.testMetadataFilesGenerationAllFiles:124 Number of generated metadata files dont match with number of checkstyle module      
missing (1): LeadingAsteriskAlign

the above error log was really long so I only added the starting part of it.

[ERROR]   XdocsJavaDocsTest.testAllCheckSectionJavaDocs:159->examineCheckSection:186 ┬╗  Error was thrown while processing src\main\java\com\puppycrawl\tools\checkstyle\checks\LeadingAsteriskAlignCheck.java

Can anyone explain me the reason for the above error?

[ERROR]   XdocsPagesTest.generateXdocContent:238 ┬╗ Parse Macro execution failed: Exception while handling moduleName: LeadingAsteriskAlignCheck propertyName: option

I've added the documentation for the property option, Idk why it is still giving an error.

[Zopsss]

I will complete the pending documentation work, is there anything else also to work on to complete this task?

And also can anybody reply me here pls? #6723 (comment)

[Zopsss]

looking for @romani @nrmancuso @rnveach @pbludov your help :)

[MANISH-K-07]

[ERROR]   XdocsJavaDocsTest.testAllCheckSectionJavaDocs:159->examineCheckSection:186 ┬╗  Error was thrown while processing src\main\java\com\puppycrawl\tools\checkstyle\checks\LeadingAsteriskAlignCheck.java

Can anyone explain me the reason for the above error?

@Zopsss , I have been facing a similar error related to docs changes in my PR #14593
Lmk if you think of a workaround.... Might help solve this for both the issues ;)

[romani]

Something wrong with documentation, please debug failed test to see what it expects

[romani]

Items

[Zopsss]

GitHub, generate site

[github-actions]

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

https://checkstyle-diff-reports.s3.us-east-2.amazonaws.com/dfddf7a_2024105357/checks.html#

https://checkstyle-diff-reports.s3.us-east-2.amazonaws.com/dfddf7a_2024105357/checks/misc/index.html#

https://checkstyle-diff-reports.s3.us-east-2.amazonaws.com/dfddf7a_2024105357/checks/misc/leadingasteriskalign.html#

[Zopsss]

I have been facing a similar error related to docs changes in my PR #14593 Lmk if you think of a workaround.... Might help solve this for both the issues ;)

Hey @MANISH-K-07 I solved this error, I don't exactly remember how I did it but I inspected the other checks to see how their documentation is written and also inspected the methods that were throwing that error. I've pushed my changes, you can check it out.

[Zopsss]

I've removed the Sun Style usage from here as we're currently not using this check in sun_style.xml, mvn clean verify was also complaining about it. As discussed here #6723 (comment) if we're going to add usage of this Module to our Sun Style Guide then we can open an separate issue in the future once the implementation of this check is done.

[MANISH-K-07]

Hey @MANISH-K-07 I solved this error, I don't exactly remember how I did it but I inspected the other checks to see how their documentation is written and also inspected the methods that were throwing that error. I've pushed my changes, you can check it out. Though I'm still getting an error which I've described here: #14643 (comment), so my solution may not be correct

Thanks for the info @Zopsss. But after digging in a bit deeper, my issue seems to be different. I'm not yet sure why but part of my doc changes are being reverted back by git at the end of validations. I suspected the problem being related to template macros and am working on resolving it as we speak !! Anyways, thanks for the help and good luck with this PR ;)

@romani , It is taking longer than expected. Any suggestions on my PR #14593 would be helpful....

[Zopsss]

GitHub, generate site 🚀

[Zopsss]

Coverage is not met. I'm facing this error:

[INFO] Loading execution data file C:\Open Source\checkstyle\target\jacoco.exec
[INFO] Analyzed bundle 'checkstyle' with 836 classes
[WARNING] Rule violated for class com.puppycrawl.tools.checkstyle.checks.LeadingAsteriskAlignCheck: lines covered ratio is 0.97, but expected minimum is 1.00
[WARNING] Rule violated for class com.puppycrawl.tools.checkstyle.checks.LeadingAsteriskAlignCheck: branches covered ratio is 0.88, but expected minimum is 1.00

I've never faced coverage error before, I'm not able to understand how can I solve it. Can you guide me through this? @romani

[Zopsss]

GitHub, generate site

[github-actions]

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

https://checkstyle-diff-reports.s3.us-east-2.amazonaws.com/49b1010_2024174011/checks.html#

https://checkstyle-diff-reports.s3.us-east-2.amazonaws.com/49b1010_2024174011/checks/misc/index.html#

https://checkstyle-diff-reports.s3.us-east-2.amazonaws.com/49b1010_2024174011/checks/misc/leadingasteriskalign.html#

[romani]

Generate html report https://github.com/checkstyle/checkstyle/wiki/How-to-run-certain-phases-and-validations#how-to-generate-ut-coverage-report and read in web how to deal with it.

If still a problem, share report in your GitHub.io to let us guide you.

[nrmancuso]

Please consider #6723 (comment), we will get this merged much faster if we create lean implementation first.

[Zopsss]

If still a problem, share report in your GitHub.io to let us guide you.

I solved it, but CI seems to be unhappy, I will look at those errors soon.

And sorry for the delay, there were many bugs in the implementation of the check so it took a little bit longer to fix them, now the check is almost ready, we only need to translate error messages into different languages.

I have a doubt though. How can we translate our error message into different languages? I saw that error messages in different languages are in encoded with some kind of format, can you guide me how can I translate the error messages into different languages with encoded format? @romani

[Zopsss]

Please consider #6723 (comment), we will get this merged much faster if we create lean implementation first.

damn I spent many hours working on this issue and now we're thinking about changing the working of it 😿. The check is almost done, just need to translate the error messages into different languages ;(

[nrmancuso]

Please consider #6723 (comment), we will get this merged much faster if we create lean implementation first.

damn I spent many hours working on this issue and now we're thinking about changing the working of it 😿. The check is almost done, just need to translate the error messages into different languages ;(

The work on this check has only begun :) You are about to spend many more hours in review + report generation and analysis + iterations on code changes and CI. The choice is up to you ultimately. Introducing new checks is one of the most difficult tasks in Checkstyle, and requires review from all maintainers.

[Zopsss]

The work on this check has only begun :) You are about to spend many more hours in review + report generation and analysis + iterations on code changes and CI. The choice is up to you ultimately. Introducing new checks is one of the most difficult tasks in Checkstyle, and requires review from all maintainers.

I'm okay with transitioning to Javadoc Check, but the new implementation might work differently from the current implementation based on the points I mentioned here: #6723 (comment) @nrmancuso

[Zopsss]

hey @romani @nrmancuso I was transitioning this check to javadoc as we discussed, I've completed the work but when running mvn clean verify I was facing this error:

[ERROR] Failed to execute goal org.apache.maven.plugins:maven-antrun-plugin:3.1.0:run (ant-phase-verify) on project checkstyle: An Ant BuildException has occured: The following error occurred while executing this line:
[ERROR] C:\Open Source\checkstyle\config\ant-phase-verify.xml:38: Unable to process files: [C:\Open Source\checkstyle\src\it\java\com\google\checkstyle\test\base\AbstractGoogleModu
leTestSupport.java, C:\Open Source\checkstyle\src\it\java\com\google\checkstyle\test\base\AbstractIndentationTestSupport.java, C:\Open Source\checkstyle\src\it\java\com\google\chec
kstyle\test\chapter2filebasic\rule21filename\OuterTypeFilenameTest.java, C:\Open Source\checkstyle\src\it\java\com\google\checkstyle\test\chapter2filebasic\rule231filetab\FileTabCh
aracterTest.java, C:\Open Source\checkstyle\src\it\java\com\google\checkstyle\test\chapter2filebasic\rule232specialescape\IllegalTokenTextTest.java, C:\Open Source\checkstyle\src\i
t\java\com\google\checkstyle\test\chapter2filebasic\rule233nonascii\AvoidEscapedUnicodeCharactersTest.java, C:\Open Source\checkstyle\src\it\java\com\google\checkstyle\test\chapter
3filestructure\rule32packagestate\LineLengthTest.java, C:\Open Source\checkstyle\src\it\java\com\google\checkstyle\test\chapter3filestructure\rule331nowildcard\AvoidStarImportTest.
java, C:\Open Source\checkstyle\src\it\java\com\google\checkstyle\test\chapter3filestructure\rule332nolinewrap\NoLineWrapTest.java, C:\Open Source\checkstyle\src\it\java\com\google
\checkstyle\test\chapter3filestructure\rule333orderingandspacing\CustomImportOrderTest.java, C:\Open Source\checkstyle\src\it\java\com\google\checkstyle\test\chapter3filestructure\

------

[ERROR] around Ant part ...<ant antfile="config/ant-phase-verify.xml" />... @ 6:50 in C:\Open Source\checkstyle\target\antrun\build-main.xml: Exception was thrown while processing C:\Open Source\checkstyle\src\main\java\com\puppycrawl\tools\checkstyle\DefaultLogger.java: Cannot invoke "com.puppycrawl.tools.checkstyle.api.DetailNode.getText()" because the return value of "com.puppycrawl.tools.checkstyle.utils.JavadocUtil.getNextSibling(com.puppycrawl.tools.checkstyle.api.DetailNode)" is null

It was showing error at getNextSibling() method inside visitJavadocToken(). The first error message was so long so I only included the starting part.

As you can see in the second error, it is saying that the return value of getNextSibling() is null, I tried to solve this by doing this:

before handling the null condition, the code was something like this:

final String commentText = "*".concat(JavadocUtil.getNextSibling(ast).getText();

After handling the null condition, it is something like this:

final DetailNode nextSibling = JavadocUtil.getNextSibling(ast);
final String commentText = nextSibling != null
                           ? "*".concat(nextSibling.getText())
                           : "";

but now mvn clean verify is giving coverage not met error. Jacoco is showing yellow diamond on nextSibling != null & : ""; lines.

All the test cases were passing without handling this null condition, from what I think I guess the getNextSibling() method will never return null value for LEADING_ASTERISK token as there will be always the TEXT token or some other token as it's children. But this might not be true.

I was thinking maybe we can handle the null condition with the code I mentioned above but then it is giving coverage not met error, I'm not able to think a condition which will satisfy the null condition. Can you give me an example in which the Leading Asterisk's next sibling will be null? so the test coverage will also be satisfied.

[Zopsss]

also some files are giving Duplicate leading asterisks error as shown here: https://app.circleci.com/pipelines/github/checkstyle/checkstyle/24816/workflows/9edc4d56-3082-4652-ab76-af7941d4735f/jobs/558613

should I fix them? or should I ignore them?

I'll fix the other errors it is showing soon, mvn clean verify didn't gave me any of this error on my local setup so I didn't knew they existed.

[nrmancuso]

should I fix them? or should I ignore them?

All CI must be green, you will need modify your code or find test cases to cover your code.

[Zopsss]

All CI must be green, you will need modify your code or find test cases to cover your code.

Yeah I know but I was asking if I should fix the Duplicate Leading Asterisk Found. errors? Checking consecutive leading asterisk is part of this check and some of the files are not following this rule, they have consecutive leading asterisks, so should I remove them or keep them?

and also can you suggest me a situation where LEADING_ASTERISK next sibling would be null? so I can cover up the test coverage. I'm not able to think a situation where this would be true >~< @nrmancuso

[Zopsss]

@nrmancuso just wanted to know that how much more time this module will require? The last generated regression report was fine, there were no false positives and the module was working fine. I also implemented the small new changes after that, based on maintainers suggestions. I want to get this PR merge before the ending of Community Bonding period ( 26th may ) so then I can start working on my project.

[nrmancuso]

@nrmancuso just wanted to know that how much more time this module will require?

There is no way of telling, I think we can finish it this weekend if we try.

I had no idea that OptionalInt existed, thanks for sharing this here.

Items:

[Zopsss]

I think we can finish it this weekend if we try.

Sure let's do it!

I had no idea that OptionalInt existed, thanks for sharing this here.

I also got to know about it from failing CIs :D

[nrmancuso]

Items:

[nrmancuso]

Last item from me:

[nrmancuso]

Very good.

[Zopsss]

Very good.

Thank you so much for guiding through this module and special thanks for today, helping me out even on weekends 😀

[Zopsss]

GitHub, generate report

[github-actions]

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

[Zopsss]

The diff report is fine, there is no false positives and module is working correctly @romani

[Zopsss]

@romani

[romani]

last update:

[romani]

10.18.0 for all

[Zopsss]

done. @romani

[romani]

ok to merge, thansk a lot for huge effort