Issue #13930: Migrate magicnumber.xml.template to use properties template #13971

relentless-pursuit · 2023-10-30T11:14:55Z

Resolves #13930

romani · 2023-11-03T00:46:06Z

@relentless-pursuit , please rebase.
Dependency update is merged

romani

Items

src/xdocs/checks/coding/magicnumber.xml

romani

I am good to merge if this works, but last question:

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

src/xdocs/checks/javadoc/atclauseorder.xml

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

romani · 2023-11-09T13:37:00Z

@relentless-pursuit , looks good to me, please make CI green.

relentless-pursuit · 2023-11-09T13:51:38Z

@relentless-pursuit , looks good to me, please make CI green.

@romani, can you help me with any insights why CI is failing?

romani · 2023-11-09T16:30:27Z

Please fix https://app.circleci.com/pipelines/github/checkstyle/checkstyle/21729/workflows/54de9a08-d1aa-4b44-b94f-9ffe29f86130/jobs/424593/artifacts

This is weird
https://ci.appveyor.com/project/Checkstyle/checkstyle/builds/48483708/job/emey2n0w2ewrl2t3#L310
[checkstyle] [ERROR] C:\projects\checkstyle\src\main\java\com\puppycrawl\tools\checkstyle\site\PropertiesMacro.java:124:5: Unexpected getter name. [SimpleAccessorNameNotation]

I don't see what can be wrong with code

https://sevntu-checkstyle.github.io/sevntu.checkstyle/apidocs/com/github/sevntu/checkstyle/checks/coding/SimpleAccessorNameNotationCheck.html

I don't see open issues on this Check https://github.com/sevntu-checkstyle/sevntu.checkstyle/issues?q=is%3Aissue+is%3Aopen+SimpleAccessorNameNotatio

No previous suppressions https://github.com/search?q=repo%3Acheckstyle%2Fcheckstyle%20SimpleAccessorNameNotation&type=code

romani · 2023-11-09T16:43:51Z

@relentless-pursuit , please report issue on this Check to sevntu issue tracker.
As workaround please make NON_BASE_TOKEN_PROPERTIES public, it is final and unmodified collection, so no harm.

rnveach · 2023-11-09T23:30:48Z

@romani Why do you think this a bug?

Unexpected getter name. [SimpleAccessorNameNotation]

According to the code, if a method is a getter, has no parameters, only is a return statement with an expression, and we identify some type of field in the expression, then that field must have the same name (ignoring casing) as the method.
Field is NON_BASE_TOKEN_PROPERTIES and method is NonBaseTokenProperties. The field has _ while the method does not.

Why isn't NON_BASE_TOKEN_PROPERTIES made pubic? It appears to be immutable.

romani · 2023-11-10T01:25:37Z

Why isn't NON_BASE_TOKEN_PROPERTIES made pubic? It appears to be immutable.

Yes.

Field is NON_BASE_TOKEN_PROPERTIES and method is NonBaseTokenProperties. The field has _ while the method does not.

It is unnatural to have getter with _ in name. Check should handle somehow fact that static final is forces to be upper cased and highly likely be named with _. But in getter better to not leak _.

romani · 2023-11-10T03:08:48Z

Circle CI:

com.puppycrawl.tools.checkstyle.site
<entry_point TYPE="method" FQNAME="com.puppycrawl.tools.checkstyle.site.PropertiesMacro void writePropertyDefaultValueCell(org.apache.maven.doxia.sink.Sink sink, java.lang.String propertyName, java.lang.reflect.Field field, java.lang.Object instance)"/>
<problem_class id="NegativelyNamedBooleanVariable" severity="ERROR" attribute_key="ERRORS_ATTRIBUTES">Negatively named boolean variable</problem_class>
Boolean variable isNonBaseTokenProp is negatively named #loc
<highlighted_element>isNonBaseTokenProp</highlighted_element>

Ok, please name it isSpecialTokenProp

relentless-pursuit · 2023-11-10T03:15:07Z

Circle CI:

com.puppycrawl.tools.checkstyle.site
<entry_point TYPE="method" FQNAME="com.puppycrawl.tools.checkstyle.site.PropertiesMacro void writePropertyDefaultValueCell(org.apache.maven.doxia.sink.Sink sink, java.lang.String propertyName, java.lang.reflect.Field field, java.lang.Object instance)"/>
<problem_class id="NegativelyNamedBooleanVariable" severity="ERROR" attribute_key="ERRORS_ATTRIBUTES">Negatively named boolean variable</problem_class>
Boolean variable isNonBaseTokenProp is negatively named #loc
<highlighted_element>isNonBaseTokenProp</highlighted_element>

Ok, please name it isSpecialTokenProp

done

romani

Ok to merge if ci pass

rnveach · 2023-11-10T03:40:52Z

src/test/java/com/puppycrawl/tools/checkstyle/internal/XdocsPagesTest.java

@@ -1037,7 +1037,8 @@ private static void validatePropertySectionPropertyEx(String fileName, String se
        if (expectedValue != null) {
            final String actualValue = columns.get(3).getTextContent().trim()
                    .replaceAll("\\s+", " ")
-                    .replaceAll("\\s,", ",");
+                    .replaceAll("\\s,", ",")
+                    .replaceAll("\\s*\\.$", "");


Why was this changed?

actual value: ARRAY_INIT, ASSIGN, DIV, ELIST, EXPR, LITERAL_NEW, METHOD_CALL, MINUS, PLUS, STAR, TYPECAST, UNARY_MINUS, UNARY_PLUS . expect value: ARRAY_INIT, ASSIGN, DIV, ELIST, EXPR, LITERAL_NEW, METHOD_CALL, MINUS, PLUS, STAR, TYPECAST, UNARY_MINUS, UNARY_PLUS

i had to trim any period at the end.

Why was this an issue in your PR? We have periods in other places.

Below is the xml for atclauseorder for target property

<tr> <td>target</td> <td>Specify block tags targeted.</td> <td>subset of tokens <a href="../../apidocs/com/puppycrawl/tools/checkstyle/api/TokenTypes.html"> TokenTypes </a> </td> <td> <a href="../../apidocs/com/puppycrawl/tools/checkstyle/api/TokenTypes.html#CLASS_DEF"> CLASS_DEF</a> , <a href="../../apidocs/com/puppycrawl/tools/checkstyle/api/TokenTypes.html#COMPACT_CTOR_DEF"> COMPACT_CTOR_DEF</a> , <a href="../../apidocs/com/puppycrawl/tools/checkstyle/api/TokenTypes.html#CTOR_DEF"> CTOR_DEF</a> , <a href="../../apidocs/com/puppycrawl/tools/checkstyle/api/TokenTypes.html#ENUM_DEF"> ENUM_DEF</a> , <a href="../../apidocs/com/puppycrawl/tools/checkstyle/api/TokenTypes.html#INTERFACE_DEF"> INTERFACE_DEF</a> , <a href="../../apidocs/com/puppycrawl/tools/checkstyle/api/TokenTypes.html#METHOD_DEF"> METHOD_DEF</a> , <a href="../../apidocs/com/puppycrawl/tools/checkstyle/api/TokenTypes.html#RECORD_DEF"> RECORD_DEF</a> , <a href="../../apidocs/com/puppycrawl/tools/checkstyle/api/TokenTypes.html#VARIABLE_DEF"> VARIABLE_DEF</a> </td> <td>6.0</td> </tr>

Below is the xml for magicnumber for property constantWaiverParentToken

<tr> <td>constantWaiverParentToken</td> <td>Specify tokens that are allowed in the AST path from the number literal to the enclosing constant definition.</td> <td>subset of tokens <a href="../../apidocs/com/puppycrawl/tools/checkstyle/api/TokenTypes.html">TokenTypes</a></td> <td> <a href="../../apidocs/com/puppycrawl/tools/checkstyle/api/TokenTypes.html#ARRAY_INIT"> ARRAY_INIT</a> , <a href="../../apidocs/com/puppycrawl/tools/checkstyle/api/TokenTypes.html#ASSIGN"> ASSIGN</a> , <a href="../../apidocs/com/puppycrawl/tools/checkstyle/api/TokenTypes.html#DIV"> DIV</a> , <a href="../../apidocs/com/puppycrawl/tools/checkstyle/api/TokenTypes.html#ELIST"> ELIST</a> , <a href="../../apidocs/com/puppycrawl/tools/checkstyle/api/TokenTypes.html#EXPR"> EXPR</a> , <a href="../../apidocs/com/puppycrawl/tools/checkstyle/api/TokenTypes.html#LITERAL_NEW"> LITERAL_NEW</a> , <a href="../../apidocs/com/puppycrawl/tools/checkstyle/api/TokenTypes.html#METHOD_CALL"> METHOD_CALL</a> , <a href="../../apidocs/com/puppycrawl/tools/checkstyle/api/TokenTypes.html#MINUS"> MINUS</a> , <a href="../../apidocs/com/puppycrawl/tools/checkstyle/api/TokenTypes.html#PLUS"> PLUS</a> , <a href="../../apidocs/com/puppycrawl/tools/checkstyle/api/TokenTypes.html#STAR"> STAR</a> , <a href="../../apidocs/com/puppycrawl/tools/checkstyle/api/TokenTypes.html#TYPECAST"> TYPECAST</a> , <a href="../../apidocs/com/puppycrawl/tools/checkstyle/api/TokenTypes.html#UNARY_MINUS"> UNARY_MINUS</a> , <a href="../../apidocs/com/puppycrawl/tools/checkstyle/api/TokenTypes.html#UNARY_PLUS"> UNARY_PLUS</a> . </td> <td>6.11</td> </tr>

I think period is not uniform.

It was reported in this PR because as far as i have observed in the files which had issues with migration in properties section, the PropertiesMacro.java has some code which adds period based on conditionals.

private static void writeTokensList(Sink sink, List<String> tokens, String tokenTypesLink) throws MacroExecutionException { for (int index = 0; index < tokens.size(); index++) { final String token = tokens.get(index); sink.rawText(INDENT_LEVEL_16); if (index != 0) { sink.text(SiteUtil.COMMA_SPACE); } writeLinkToToken(sink, tokenTypesLink, token); } if (tokens.isEmpty()) { sink.rawText(CODE_START); sink.text("empty"); sink.rawText(CODE_END); } else { sink.rawText(INDENT_LEVEL_18); sink.rawText(SiteUtil.DOT); sink.rawText(INDENT_LEVEL_14); } }

Please share output on how they are failing. We need to show such details with @rnveach to let him understand issue

Please share output on how they are failing. We need to show such details with @rnveach to let him understand issue

I have already shared the failures. Re-sharing it again and adding some context to it.

for magicnumber.xml,

actual value: ARRAY_INIT, ASSIGN, DIV, ELIST, EXPR, LITERAL_NEW, METHOD_CALL, MINUS, PLUS, STAR, TYPECAST, UNARY_MINUS, UNARY_PLUS . expect value: ARRAY_INIT, ASSIGN, DIV, ELIST, EXPR, LITERAL_NEW, METHOD_CALL, MINUS, PLUS, STAR, TYPECAST, UNARY_MINUS, UNARY_PLUS

for multiplestringliterals.xml

multiplestringliterals.xml section 'MultipleStringLiterals' should have the value for ignoreOccurrenceContext expected: ANNOTATION but was : ANNOTATION . Expected :ANNOTATION Actual :ANNOTATION .

This is primarily happening because we add a . in the below code:

private static void writeTokensList(Sink sink, List<String> tokens, String tokenTypesLink) throws MacroExecutionException { for (int index = 0; index < tokens.size(); index++) { final String token = tokens.get(index); sink.rawText(INDENT_LEVEL_16); if (index != 0) { sink.text(SiteUtil.COMMA_SPACE); } writeLinkToToken(sink, tokenTypesLink, token); } if (tokens.isEmpty()) { sink.rawText(CODE_START); sink.text("empty"); sink.rawText(CODE_END); } else { sink.rawText(INDENT_LEVEL_18); sink.rawText(SiteUtil.DOT); sink.rawText(INDENT_LEVEL_14); } }

And, this particular piece of code from XdocsPagesTest.java gets the content by of a node and all its descendants added by PropertiesMacro.java in form of XML.

if (expectedValue != null) { final String actualValue = columns.get(3).getTextContent().trim() .replaceAll("\\s+", " ") .replaceAll("\\s,", ",");

To illustrate further, when actualValue is retrieved it sees a . and registers it while expectedValue doesn't, giving assertion error.

<tr> <td>constantWaiverParentToken</td> <td>Specify tokens that are allowed in the AST path from the number literal to the enclosing constant definition.</td> <td>subset of tokens <a href="../../apidocs/com/puppycrawl/tools/checkstyle/api/TokenTypes.html">TokenTypes</a></td> <td> <a href="../../apidocs/com/puppycrawl/tools/checkstyle/api/TokenTypes.html#ARRAY_INIT"> ARRAY_INIT</a> , <a href="../../apidocs/com/puppycrawl/tools/checkstyle/api/TokenTypes.html#ASSIGN"> ASSIGN</a> , <a href="../../apidocs/com/puppycrawl/tools/checkstyle/api/TokenTypes.html#DIV"> DIV</a> , <a href="../../apidocs/com/puppycrawl/tools/checkstyle/api/TokenTypes.html#ELIST"> ELIST</a> , <a href="../../apidocs/com/puppycrawl/tools/checkstyle/api/TokenTypes.html#EXPR"> EXPR</a> , <a href="../../apidocs/com/puppycrawl/tools/checkstyle/api/TokenTypes.html#LITERAL_NEW"> LITERAL_NEW</a> , <a href="../../apidocs/com/puppycrawl/tools/checkstyle/api/TokenTypes.html#METHOD_CALL"> METHOD_CALL</a> , <a href="../../apidocs/com/puppycrawl/tools/checkstyle/api/TokenTypes.html#MINUS"> MINUS</a> , <a href="../../apidocs/com/puppycrawl/tools/checkstyle/api/TokenTypes.html#PLUS"> PLUS</a> , <a href="../../apidocs/com/puppycrawl/tools/checkstyle/api/TokenTypes.html#STAR"> STAR</a> , <a href="../../apidocs/com/puppycrawl/tools/checkstyle/api/TokenTypes.html#TYPECAST"> TYPECAST</a> , <a href="../../apidocs/com/puppycrawl/tools/checkstyle/api/TokenTypes.html#UNARY_MINUS"> UNARY_MINUS</a> , <a href="../../apidocs/com/puppycrawl/tools/checkstyle/api/TokenTypes.html#UNARY_PLUS"> UNARY_PLUS</a> . </td> <td>6.11</td> </tr>

Finally, this pattern holds true for all xml for any specific property as such:

<td>subset of tokens <a href="../../apidocs/com/puppycrawl/tools/checkstyle/api/TokenTypes.html">TokenTypes</a></td> <td> // some links . </td>

As a proof of concept, you can try migrating atclauseordr.xml and find this failure too. Currently, atclauseorder.xml isn't migrated.

@rnveach i have pushed a new patchset which removes the need to trim . . Infact, it removes . from the xml.

The latest difference doesn't have a removed . and I see an added . in the old version now, so it seems this PR was adding a period that was never being made before. That is what was different and required the extra change in the test.

this pattern holds true for all xml for any specific property as such

This is not correct, as we don't see a bigger difference in the new PR. I now see there is a difference between tokens property, while the areas without a period are not tokens properties, but are properties that specify a list of tokens.

Now this is making more sense to me.

This is now done, I do see 1 more change.

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

config/checkstyle-non-main-files-suppressions.xml

romani · 2023-11-10T18:17:33Z

Semaphore is restarted

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

rnveach · 2023-11-14T13:43:04Z

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

+            if (isSpecialTokenProp && !CURLY_BRACKET.equals(defaultValue)) {
+                final List<String> defaultValuesList =
+                        Arrays.asList(COMMA_SPACE_PATTERN.split(defaultValue));
+                writeTokensListDotLess(sink, defaultValuesList, SiteUtil.PATH_TO_TOKEN_TYPES);


Suggested change

writeTokensListDotLess(sink, defaultValuesList, SiteUtil.PATH_TO_TOKEN_TYPES);

writeTokensList(sink, defaultValuesList, SiteUtil.PATH_TO_TOKEN_TYPES, false);

Add a boolean onto the end of writeTokensList, and use that to control the period. Other writeTokensList calls should use true. Remove writeTokensListDotLess.

@rnveach, the idea of a introducing a boolean variable was insightful. thank you @romani for the help.

@relentless-pursuit I am against duplicating chunks of code if we can achieve what we need with simple changes.

…rties template

romani · 2023-11-14T16:36:32Z

Rebased one more time to fix problem caused by non compliant releasenotes

romani · 2023-11-14T17:12:12Z

@rnveach , CI is green, please finish review.

relentless-pursuit force-pushed the macrosMagicNumber branch from 11b597f to f3edc23 Compare October 30, 2023 11:49

relentless-pursuit force-pushed the macrosMagicNumber branch from f3edc23 to de517e5 Compare November 3, 2023 04:06

romani requested changes Nov 3, 2023

View reviewed changes

src/xdocs/checks/coding/magicnumber.xml Outdated Show resolved Hide resolved

relentless-pursuit force-pushed the macrosMagicNumber branch from de517e5 to 05799b6 Compare November 7, 2023 11:02

romani requested changes Nov 7, 2023

View reviewed changes

src/main/java/com/puppycrawl/tools/checkstyle/site/PropertiesMacro.java Outdated Show resolved Hide resolved

relentless-pursuit mentioned this pull request Nov 7, 2023

Issue #13893: Migrate illegaltype.xml.template to use properties temp… #13993

Merged

relentless-pursuit force-pushed the macrosMagicNumber branch from 05799b6 to 468bf1e Compare November 7, 2023 15:29

relentless-pursuit commented Nov 7, 2023

View reviewed changes

src/xdocs/checks/javadoc/atclauseorder.xml Outdated Show resolved Hide resolved

relentless-pursuit force-pushed the macrosMagicNumber branch 3 times, most recently from 10ff602 to 9d404cd Compare November 7, 2023 19:36

rnveach requested changes Nov 7, 2023

View reviewed changes

src/main/java/com/puppycrawl/tools/checkstyle/site/PropertiesMacro.java Outdated Show resolved Hide resolved

relentless-pursuit force-pushed the macrosMagicNumber branch from 9d404cd to efc43af Compare November 9, 2023 12:53

relentless-pursuit force-pushed the macrosMagicNumber branch from efc43af to 3be4878 Compare November 9, 2023 13:46

relentless-pursuit force-pushed the macrosMagicNumber branch 2 times, most recently from d8c98bf to dce5087 Compare November 10, 2023 02:49

relentless-pursuit force-pushed the macrosMagicNumber branch 2 times, most recently from 0867b15 to 72bb03c Compare November 10, 2023 03:14

romani approved these changes Nov 10, 2023

View reviewed changes

romani assigned rnveach Nov 10, 2023

rnveach requested changes Nov 10, 2023

View reviewed changes

relentless-pursuit force-pushed the macrosMagicNumber branch 2 times, most recently from 9afdda0 to e4d8302 Compare November 10, 2023 05:37

relentless-pursuit mentioned this pull request Nov 11, 2023

Issue #13893: Remove issue reference from checkstyle-non-main-files-suppressions.xml #14017

Merged

romani force-pushed the macrosMagicNumber branch from e4d8302 to bac4564 Compare November 11, 2023 14:00

rnveach reviewed Nov 11, 2023

View reviewed changes

src/main/java/com/puppycrawl/tools/checkstyle/site/PropertiesMacro.java Outdated Show resolved Hide resolved

relentless-pursuit force-pushed the macrosMagicNumber branch 2 times, most recently from 1a934ee to 5d858d9 Compare November 14, 2023 06:13

rnveach requested changes Nov 14, 2023

View reviewed changes

romani force-pushed the macrosMagicNumber branch from 5d858d9 to b0609eb Compare November 14, 2023 14:33

Issue checkstyle#13930: Migrate magicnumber.xml.template to use prope…

49b7ed9

…rties template

romani force-pushed the macrosMagicNumber branch from b0609eb to 49b7ed9 Compare November 14, 2023 16:34

rnveach approved these changes Nov 15, 2023

View reviewed changes

rnveach assigned romani and unassigned rnveach Nov 15, 2023

romani merged commit 439d69c into checkstyle:master Nov 15, 2023
113 checks passed

relentless-pursuit deleted the macrosMagicNumber branch December 4, 2023 04:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Issue #13930: Migrate magicnumber.xml.template to use properties template #13971

Issue #13930: Migrate magicnumber.xml.template to use properties template #13971

relentless-pursuit commented Oct 30, 2023 •

edited

romani commented Nov 3, 2023 •

edited

romani left a comment

romani left a comment •

edited

romani commented Nov 9, 2023

relentless-pursuit commented Nov 9, 2023 •

edited

romani commented Nov 9, 2023 •

edited

romani commented Nov 9, 2023

rnveach commented Nov 9, 2023

romani commented Nov 10, 2023

romani commented Nov 10, 2023

relentless-pursuit commented Nov 10, 2023

romani left a comment

rnveach Nov 10, 2023

relentless-pursuit Nov 10, 2023 •

edited by romani

rnveach Nov 10, 2023

relentless-pursuit Nov 10, 2023

relentless-pursuit Nov 10, 2023

romani Nov 14, 2023

relentless-pursuit Nov 14, 2023 •

edited

relentless-pursuit Nov 14, 2023 •

edited

rnveach Nov 14, 2023

rnveach Nov 14, 2023

romani commented Nov 10, 2023

rnveach Nov 14, 2023

romani Nov 14, 2023

relentless-pursuit Nov 14, 2023

rnveach Nov 14, 2023

romani commented Nov 14, 2023

romani commented Nov 14, 2023

	writeTokensListDotLess(sink, defaultValuesList, SiteUtil.PATH_TO_TOKEN_TYPES);
	writeTokensList(sink, defaultValuesList, SiteUtil.PATH_TO_TOKEN_TYPES, false);

Issue #13930: Migrate magicnumber.xml.template to use properties template #13971

Issue #13930: Migrate magicnumber.xml.template to use properties template #13971

Conversation

relentless-pursuit commented Oct 30, 2023 • edited

romani commented Nov 3, 2023 • edited

romani left a comment

Choose a reason for hiding this comment

romani left a comment • edited

Choose a reason for hiding this comment

romani commented Nov 9, 2023

relentless-pursuit commented Nov 9, 2023 • edited

romani commented Nov 9, 2023 • edited

romani commented Nov 9, 2023

rnveach commented Nov 9, 2023

romani commented Nov 10, 2023

romani commented Nov 10, 2023

relentless-pursuit commented Nov 10, 2023

romani left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

relentless-pursuit Nov 10, 2023 • edited by romani

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

relentless-pursuit Nov 14, 2023 • edited

Choose a reason for hiding this comment

relentless-pursuit Nov 14, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

romani commented Nov 10, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

romani commented Nov 14, 2023

romani commented Nov 14, 2023

relentless-pursuit commented Oct 30, 2023 •

edited

romani commented Nov 3, 2023 •

edited

romani left a comment •

edited

relentless-pursuit commented Nov 9, 2023 •

edited

romani commented Nov 9, 2023 •

edited

relentless-pursuit Nov 10, 2023 •

edited by romani

relentless-pursuit Nov 14, 2023 •

edited

relentless-pursuit Nov 14, 2023 •

edited