Issue #13934: Version updated from 3.2 to 3.4 #13945

Rohanraj123 · 2023-10-24T10:07:46Z

src/main/java/com/puppycrawl/tools/checkstyle/checks/coding/ReturnCountCheck.java

rnveach · 2023-10-24T14:27:46Z

From issue: https://github.com/checkstyle/checkstyle/pull/13932/files#r1368105661

Property was format originally. It was changed from 3.4 to 3.2 originally.

https://github.com/checkstyle/checkstyle/blob/release3_4/src/checkstyle/com/puppycrawl/tools/checkstyle/checks/AbstractFormatCheck.java

3.2 check above didn't specify format or extend AbstractFormatCheck.

It did extend AbstractFormatCheck in 3.4 .
https://github.com/checkstyle/checkstyle/blob/release3_4/src/checkstyle/com/puppycrawl/tools/checkstyle/checks/coding/ReturnCountCheck.java

But it did in 3.4 .
https://github.com/checkstyle/checkstyle/blob/release3_4/src/checkstyle/com/puppycrawl/tools/checkstyle/checks/AbstractFormatCheck.java

Rohanraj123 · 2023-10-25T08:40:05Z

@romani Sir can you please review the changes and please provide me some enhancements if required.

rnveach · 2023-10-25T15:26:32Z

@Rohanraj123 You have not resolved all my review items yet.

romani · 2023-10-25T22:28:33Z

@Rohanraj123 , please redirect all questions to @rnveach , he is first reviewer for this PR.

Rohanraj123 · 2023-10-26T07:04:32Z

@Rohanraj123 You have not resolved all my review items yet.

@rnveach Sir, can you please one more time define the doables so that i can keep them in mind while making the changes. Would be helpful for my learning as well.

rnveach · 2023-10-26T14:20:13Z

@Rohanraj123 Where is the confusion with the information you were given at #13934 (comment) and my review items at #13945 (comment) , #13945 (comment) , and #13945 (comment) .

rnveach · 2023-10-26T15:24:51Z

@Rohanraj123
The line at

checkstyle/src/main/java/com/puppycrawl/tools/checkstyle/site/SiteUtil.java

Line 702 in 0322ba1

else if (SUPER_CLASS_PROPERTIES_JAVADOCS.containsKey(propertyName)

needs to be removed.

With it gone, it will re-generate the XML files with the correct versions. I recommend to start with a clean master first.

Rohanraj123 · 2023-10-27T06:17:51Z

@Rohanraj123 The line at

checkstyle/src/main/java/com/puppycrawl/tools/checkstyle/site/SiteUtil.java

Line 702 in 0322ba1

else if (SUPER_CLASS_PROPERTIES_JAVADOCS.containsKey(propertyName)

needs to be removed.
With it gone, it will re-generate the XML files with the correct versions. I recommend to start with a clean master first.

Sure sir Let me do the changes

Rohanraj123 · 2023-10-28T05:28:18Z

@rnveach Sir, removing the line of code you were reffering giving error : ```

[ERROR] com.puppycrawl.tools.checkstyle.internal.XdocsPagesTest -- Time elapsed: 2.495 s <<< ERROR!
java.lang.NullPointerException: Cannot invoke "com.puppycrawl.tools.checkstyle.api.DetailNode.getChildren()" because "node" is null

rnveach · 2023-10-28T14:00:07Z

Update the PR. I would need to see what is being done and the full stack trace.

rnveach · 2023-10-30T13:31:43Z

@Rohanraj123 The exception being generated does not provide us information besides its failing to read a since version in a file. We need more information. I recommend start debugging and see why it is failing.

Add a second commit titled something like minor: add file name to xdoc generator exception.

Add the following to the commit:

diff --git a/src/test/java/com/puppycrawl/tools/checkstyle/internal/utils/XdocGenerator.java b/src/test/java/com/puppycrawl/tools/checkstyle/internal/utils/XdocGenerator.java
index fdfeadb..6ae2ff8 100644
--- a/src/test/java/com/puppycrawl/tools/checkstyle/internal/utils/XdocGenerator.java
+++ b/src/test/java/com/puppycrawl/tools/checkstyle/internal/utils/XdocGenerator.java
@@ -67,6 +67,12 @@
                     String.valueOf(StandardCharsets.UTF_8))) {
                 parser.parse(reader, sink);
             }
+            // -@cs[IllegalCatch] There is no other way to deliver filename that was under
+            // processing.
+            catch (Exception ex) {
+                throw new IllegalStateException("Exception was thrown while processing "
+                        + inputFile.getPath(), ex);
+            }
             finally {
                 sink.close();
             }

This will ensure we see the file causing the issue. I picked this spot from looking at the stack trace.

In addition, fix up your first commit to remove the unnecessary changes. Keep as clean as you can (meaning similar to the original code).

Rohanraj123 · 2023-10-31T09:40:18Z

Okay sir let me do the changes

Rohanraj123 · 2023-11-03T14:17:59Z

@rnveach Sir, I have added the catch check in XdocGenerator file. And squash them in one commit. When i tried to run mvn clean verify , It says that Exception arrives in src\xdocs\checks\whitespace\filetabcharacter.xml.template .
Sir it might take some time for me to work on this issue because my university exams are going on. So Please try to provide me some time.

romani

Ok to merge

rnveach · 2023-11-03T15:28:04Z

@Rohanraj123 No worries. You have some time. I am looking into the issue a bit more to understand things.

@romani CI is failing, so is too soon to say it is ready for merging.

rnveach · 2023-11-03T15:41:49Z

I was digging through the error and I think I understand the logic of why the code we removed is there. filetabcharacter is an FileSetCheck. The error for the since version is because it is looking for the javadoc of fileExtensions and not finding it since it is in the parent class and has no version number defined there. This is a property for all FileSetCheck, so I assume the reasoning for the removed code was that this class has been here long enough that all modules that use it, should have had it from the start. However, this logic starts to fall apart if we add a new property in the future to any abstract checks that are already in existence.

We can actually see this for 2 of the overrides we did for SINCE_VERSION_FOR_INHERITED_PROPERTY for 2 same inherited properties:

checkstyle/src/main/java/com/puppycrawl/tools/checkstyle/site/SiteUtil.java

Lines 165 to 166 in 1282fe6

    
           Map.entry("MissingDeprecatedCheck.violateExecutionOnNonTightHtml", "8.24"), 
        
           Map.entry("NonEmptyAtclauseDescriptionCheck.violateExecutionOnNonTightHtml", "8.3"),

And I think that's where the solution lies, we need to add these properties to this global override field. So I would expect us to add a few <ModuleName>.fileExtensions to the list with each module's specific version that it defines. Most likely all may be the same as Module's own version number.

Ideally, we could remove the need for this list if we did version comparing between the version the module was introduced versus the version the inherited property was introduced and taking the newest of the 2. We are tracking inherited properties directly through SUPER_CLASS_PROPERTIES_JAVADOCS. I would expect this to be done in another PR. @romani If you agree we should create the issue for this.

Edit: We would still need the override to fall back on if we changed extension of the module after it was introduced, like changing AbstractCheck to AbstractJavadocCheck. We have a few modules we are looking to do that to still. Only way to avoid the override is if we put a since version on the class hierarchy.

romani · 2023-11-08T02:44:35Z

@Rohanraj123, if you think that this PR becomes a bit complicated to finish.
It is completely fine to pause it or abandon it and switch to easier issues to get experience a bit more before handling complicated.
We want you to be productive and not be burned by heavy problems. Sorry that we didn't predict that it is not easy issue.

Rohanraj123 · 2023-11-10T17:32:58Z

Sure @rnveach Sir , letme add .fileextensions first. Then we will discuss furthur

romani · 2023-11-11T02:06:23Z

ci failure:

[INFO] Running com.puppycrawl.tools.checkstyle.internal.XdocsPagesTest
[ERROR] Tests run: 1, Failures: 0, Errors: 1, Skipped: 0, Time elapsed: 2.296 s <<< FAILURE! -- in com.puppycrawl.tools.checkstyle.internal.XdocsPagesTest
[ERROR] com.puppycrawl.tools.checkstyle.internal.XdocsPagesTest -- Time elapsed: 2.296 s <<< ERROR!
java.lang.IllegalStateException: Exception was thrown while processing src/xdocs/checks/whitespace/typecastparenpad.xml.template
  at org.junit.platform.launcher.core.EngineExecutionOrchestrator.execute(EngineExecutionOrchestrator.java:127)
 ....
  at org.apache.maven.surefire.booter.ForkedBooter.main(ForkedBooter.java:495)
Caused by: java.lang.NullPointerException
  at com.puppycrawl.tools.checkstyle.utils.JavadocUtil.getFirstChild(JavadocUtil.java:220)
  at com.puppycrawl.tools.checkstyle.utils.JavadocUtil.findFirstToken(JavadocUtil.java:200)
  at com.puppycrawl.tools.checkstyle.site.SiteUtil.getSinceVersionFromJavadoc(SiteUtil.java:727)
  at com.puppycrawl.tools.checkstyle.site.SiteUtil.getSinceVersion(SiteUtil.java:707)
  at com.puppycrawl.tools.checkstyle.site.PropertiesMacro.writePropertySinceVersionCell(PropertiesMacro.java:492)
  at com.puppycrawl.tools.checkstyle.site.PropertiesMacro.writePropertyRow(PropertiesMacro.java:233)
  at com.puppycrawl.tools.checkstyle.site.PropertiesMacro.writeTablePropertiesRows(PropertiesMacro.java:187)
  at com.puppycrawl.tools.checkstyle.site.PropertiesMacro.writePropertiesTable(PropertiesMacro.java:130)
  at com.puppycrawl.tools.checkstyle.site.PropertiesMacro.execute(PropertiesMacro.java:102)
  at org.apache.maven.doxia.parser.AbstractParser.executeMacro(AbstractParser.java:145)
  at com.puppycrawl.tools.checkstyle.site.XdocsTemplateParser.processMacroEnd(XdocsTemplateParser.java:187)
  at com.puppycrawl.tools.checkstyle.site.XdocsTemplateParser.handleEndTag(XdocsTemplateParser.java:116)
  at org.apache.maven.doxia.parser.AbstractXmlParser.parseXml(AbstractXmlParser.java:238)
  at org.apache.maven.doxia.parser.AbstractXmlParser.parse(AbstractXmlParser.java:156)
  at org.apache.maven.doxia.parser.XhtmlBaseParser.parse(XhtmlBaseParser.java:92)
  at org.apache.maven.doxia.module.xdoc.XdocParser.parse(XdocParser.java:113)
  at com.puppycrawl.tools.checkstyle.site.XdocsTemplateParser.parse(XdocsTemplateParser.java:76)
  at org.apache.maven.doxia.parser.AbstractParser.parse(AbstractParser.java:207)
  at com.puppycrawl.tools.checkstyle.internal.utils.XdocGenerator.generateXdocContent(XdocGenerator.java:68)
  ... 62 more

Rohanraj123 · 2023-11-11T11:37:27Z

ci failure:

[INFO] Running com.puppycrawl.tools.checkstyle.internal.XdocsPagesTest
[ERROR] Tests run: 1, Failures: 0, Errors: 1, Skipped: 0, Time elapsed: 2.296 s <<< FAILURE! -- in com.puppycrawl.tools.checkstyle.internal.XdocsPagesTest
[ERROR] com.puppycrawl.tools.checkstyle.internal.XdocsPagesTest -- Time elapsed: 2.296 s <<< ERROR!
java.lang.IllegalStateException: Exception was thrown while processing src/xdocs/checks/whitespace/typecastparenpad.xml.template
  at org.junit.platform.launcher.core.EngineExecutionOrchestrator.execute(EngineExecutionOrchestrator.java:127)
 ....
  at org.apache.maven.surefire.booter.ForkedBooter.main(ForkedBooter.java:495)
Caused by: java.lang.NullPointerException
  at com.puppycrawl.tools.checkstyle.utils.JavadocUtil.getFirstChild(JavadocUtil.java:220)
  at com.puppycrawl.tools.checkstyle.utils.JavadocUtil.findFirstToken(JavadocUtil.java:200)
  at com.puppycrawl.tools.checkstyle.site.SiteUtil.getSinceVersionFromJavadoc(SiteUtil.java:727)
  at com.puppycrawl.tools.checkstyle.site.SiteUtil.getSinceVersion(SiteUtil.java:707)
  at com.puppycrawl.tools.checkstyle.site.PropertiesMacro.writePropertySinceVersionCell(PropertiesMacro.java:492)
  at com.puppycrawl.tools.checkstyle.site.PropertiesMacro.writePropertyRow(PropertiesMacro.java:233)
  at com.puppycrawl.tools.checkstyle.site.PropertiesMacro.writeTablePropertiesRows(PropertiesMacro.java:187)
  at com.puppycrawl.tools.checkstyle.site.PropertiesMacro.writePropertiesTable(PropertiesMacro.java:130)
  at com.puppycrawl.tools.checkstyle.site.PropertiesMacro.execute(PropertiesMacro.java:102)
  at org.apache.maven.doxia.parser.AbstractParser.executeMacro(AbstractParser.java:145)
  at com.puppycrawl.tools.checkstyle.site.XdocsTemplateParser.processMacroEnd(XdocsTemplateParser.java:187)
  at com.puppycrawl.tools.checkstyle.site.XdocsTemplateParser.handleEndTag(XdocsTemplateParser.java:116)
  at org.apache.maven.doxia.parser.AbstractXmlParser.parseXml(AbstractXmlParser.java:238)
  at org.apache.maven.doxia.parser.AbstractXmlParser.parse(AbstractXmlParser.java:156)
  at org.apache.maven.doxia.parser.XhtmlBaseParser.parse(XhtmlBaseParser.java:92)
  at org.apache.maven.doxia.module.xdoc.XdocParser.parse(XdocParser.java:113)
  at com.puppycrawl.tools.checkstyle.site.XdocsTemplateParser.parse(XdocsTemplateParser.java:76)
  at org.apache.maven.doxia.parser.AbstractParser.parse(AbstractParser.java:207)
  at com.puppycrawl.tools.checkstyle.internal.utils.XdocGenerator.generateXdocContent(XdocGenerator.java:68)
  ... 62 more

I fixed this CI failure @romani Sir.

rnveach

Should be final change

rnveach · 2023-11-12T00:44:04Z

src/main/java/com/puppycrawl/tools/checkstyle/site/SiteUtil.java

-        else if (SUPER_CLASS_PROPERTIES_JAVADOCS.containsKey(propertyName)
-                || TOKENS.equals(propertyName)
-                || JAVADOC_TOKENS.equals(propertyName)) {
+        else if (TOKENS.equals(propertyName) || JAVADOC_TOKENS.equals(propertyName)) {


Suggested change

else if (TOKENS.equals(propertyName) || JAVADOC_TOKENS.equals(propertyName)) {

else if (TOKENS.equals(propertyName)

|| JAVADOC_TOKENS.equals(propertyName)) {

restore original lining and indentation

@rnveach Even After making these changes it is not updating sinceVersion.

@Rohanraj123 , restore formatting only

else if (TOKENS.equals(propertyName) || JAVADOC_TOKENS.equals(propertyName)) {

code looks good.

Even After making these changes it is not updating sinceVersion.

What do you mean?
It was updated at https://github.com/checkstyle/checkstyle/pull/13945/files#diff-eaff5fa3cb101145757a2bd9ebe2e805e8fd8057ae27f9e21ef91900cb7938cc

Okay got it thanks !

@Rohanraj123 , restore formatting only

else if (TOKENS.equals(propertyName) || JAVADOC_TOKENS.equals(propertyName)) {

did it ! @romani Sir

rnveach requested changes Oct 24, 2023

View reviewed changes

src/main/java/com/puppycrawl/tools/checkstyle/checks/coding/ReturnCountCheck.java Outdated Show resolved Hide resolved

rnveach assigned romani Oct 24, 2023

rnveach requested changes Oct 24, 2023

View reviewed changes

src/main/java/com/puppycrawl/tools/checkstyle/checks/coding/ReturnCountCheck.java Outdated Show resolved Hide resolved

romani approved these changes Nov 3, 2023

View reviewed changes

romani assigned rnveach and unassigned romani Nov 3, 2023

rnveach added the awaiting changes label Nov 7, 2023

rnveach requested changes Nov 12, 2023

View reviewed changes

rnveach removed the awaiting changes label Nov 12, 2023

Issue #13934: Version Update from 3.2 to 3.4

a4ccb69

rnveach approved these changes Nov 13, 2023

View reviewed changes

rnveach merged commit b5d2509 into checkstyle:master Nov 13, 2023
113 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Issue #13934: Version updated from 3.2 to 3.4 #13945

Issue #13934: Version updated from 3.2 to 3.4 #13945

Rohanraj123 commented Oct 24, 2023

rnveach commented Oct 24, 2023

Rohanraj123 commented Oct 25, 2023

rnveach commented Oct 25, 2023

romani commented Oct 25, 2023

Rohanraj123 commented Oct 26, 2023

rnveach commented Oct 26, 2023

rnveach commented Oct 26, 2023 •

edited

Rohanraj123 commented Oct 27, 2023

Rohanraj123 commented Oct 28, 2023 •

edited

rnveach commented Oct 28, 2023

rnveach commented Oct 30, 2023 •

edited

Rohanraj123 commented Oct 31, 2023 •

edited by romani

Rohanraj123 commented Nov 3, 2023

romani left a comment

rnveach commented Nov 3, 2023

rnveach commented Nov 3, 2023 •

edited

romani commented Nov 8, 2023 •

edited

Rohanraj123 commented Nov 10, 2023 •

edited by romani

romani commented Nov 11, 2023

Rohanraj123 commented Nov 11, 2023

rnveach left a comment

rnveach Nov 12, 2023 •

edited

Rohanraj123 Nov 12, 2023

Rohanraj123 Nov 12, 2023

romani Nov 12, 2023

romani Nov 12, 2023

rnveach Nov 12, 2023

Rohanraj123 Nov 13, 2023

Rohanraj123 Nov 13, 2023

	else if (TOKENS.equals(propertyName) \|\| JAVADOC_TOKENS.equals(propertyName)) {
	else if (TOKENS.equals(propertyName)
	\|\| JAVADOC_TOKENS.equals(propertyName)) {

Issue #13934: Version updated from 3.2 to 3.4 #13945

Issue #13934: Version updated from 3.2 to 3.4 #13945

Conversation

Rohanraj123 commented Oct 24, 2023

rnveach commented Oct 24, 2023

Rohanraj123 commented Oct 25, 2023

rnveach commented Oct 25, 2023

romani commented Oct 25, 2023

Rohanraj123 commented Oct 26, 2023

rnveach commented Oct 26, 2023

rnveach commented Oct 26, 2023 • edited

Rohanraj123 commented Oct 27, 2023

Rohanraj123 commented Oct 28, 2023 • edited

rnveach commented Oct 28, 2023

rnveach commented Oct 30, 2023 • edited

Rohanraj123 commented Oct 31, 2023 • edited by romani

Rohanraj123 commented Nov 3, 2023

romani left a comment

Choose a reason for hiding this comment

rnveach commented Nov 3, 2023

rnveach commented Nov 3, 2023 • edited

romani commented Nov 8, 2023 • edited

Rohanraj123 commented Nov 10, 2023 • edited by romani

romani commented Nov 11, 2023

Rohanraj123 commented Nov 11, 2023

rnveach left a comment

Choose a reason for hiding this comment

rnveach Nov 12, 2023 • edited

Choose a reason for hiding this comment

Rohanraj123 Nov 12, 2023

Choose a reason for hiding this comment

Rohanraj123 Nov 12, 2023

Choose a reason for hiding this comment

romani Nov 12, 2023

Choose a reason for hiding this comment

romani Nov 12, 2023

Choose a reason for hiding this comment

rnveach Nov 12, 2023

Choose a reason for hiding this comment

Rohanraj123 Nov 13, 2023

Choose a reason for hiding this comment

Rohanraj123 Nov 13, 2023

Choose a reason for hiding this comment

rnveach commented Oct 26, 2023 •

edited

Rohanraj123 commented Oct 28, 2023 •

edited

rnveach commented Oct 30, 2023 •

edited

Rohanraj123 commented Oct 31, 2023 •

edited by romani

rnveach commented Nov 3, 2023 •

edited

romani commented Nov 8, 2023 •

edited

Rohanraj123 commented Nov 10, 2023 •

edited by romani

rnveach Nov 12, 2023 •

edited