Issue #13399: Implement module properties macro by stoyanK7 · Pull Request #13407 · checkstyle/checkstyle

stoyanK7 · 2023-07-18T18:33:59Z

Resolves #13399

Creates PropertiesMacro which is responsible for creating the table with 5 properties - name, description, type, default value, since. Lots of code has been taken from XdocsPagesTest as it already does what we want to.

The way the macro works is:

Call ClassAndPropertiesSettersJavadocScraper to scrape all the setter javadocs of parent classes - for example AbstractNameCheck which holds properties such as applyToPublic, applyToProtected. Properties that are not specified in checks classes but are inherited. This step is done only once when the macro is executed for the first time.
Process the module given to the macro - gather the class and setters javadocs from the file
Write the header row - name, description, type, default value, since
Get a set of the properties via the class property descriptors
Filter this set of properties - remove ones that are not supposed to be documented and add ones that are supposed to be - tokens, fileExtensions, etc..
Iterate trough this set of properties and write the 5 columns
- Property name is coming from the set
- Description is scraped off the setter javadoc. A description is any text from the beginning of the Javadoc until we meet a single asterisk on a line. For example:
```
/**
 * This over here is a setter description. The alone asterisk below is what says the description is over
 *   <---- end of description
 * @param ..l.
 */
```
- Type comes from the methods taken from XdocsPagesTest
- Same for default value - from XdocsPagesTest
- Since version comes from one of 3 places(in order)
  1. Exception list - This is a list where properties are inherited from a parent but the property was introduced in a version after the creation of the check
  2. If property is inerited from parent and not part of module file - use @since xxx from class javadoc
  3. Else use the @since xxx version from property setter Javadoc

romani · 2023-08-08T12:42:18Z

@stoyanK7 , please share some hints of this PR status and challenges that you facing.

nrmancuso · 2023-08-17T19:45:25Z

@stoyanK7 let's get to reviewable state

romani · 2023-08-26T14:22:22Z

@stoyanK7 , please keep fixing CI failures - https://ci.appveyor.com/project/Checkstyle/checkstyle/builds/47874621/job/t52fqlfkkwhbsbsm#L310 to be ready for merge at any time.

nrmancuso

@stoyanK7, think like a reviewer and do first pass on your own PRs, this will shorten feedback loop for non-obvious items and get PRs merged faster :)

Items:

romani

items:

nrmancuso

Few more:

nrmancuso · 2023-08-27T14:17:16Z

+                result = '"' + value.toString().replace("\n", "\\n").replace("\t", "\\t")
+                        .replace("\r", "\\r").replace("\f", "\\f") + '"';


Please share an example of a check that requires this replacement. Also, is it safe to just always assume that we need to do \ -> \\?

This method and others I have taken as-is from XdocsPagesTest - getProperties, fixCapturedProperties, getType, getDefaultValue, getPatternArrayPropertyValue, getStringArrayPropertyValue, getIntArrayPropertyValue, getFieldClass, subtractTokens.

To answer your question - running the code from XdocsPagesTest, JavadocStyle.endOfSentenceFormat pops up

Also, is it safe to just always assume that we need to do \ -> \

I think so. Makes value readable:

Value before

([.?!][ <])|([.?!]$)

Value after

([.?!][ \t\n\r\f<])|([.?!]$)

nrmancuso · 2023-08-27T14:51:52Z

+        final Object instance = SiteUtil.getModuleInstance(moduleName);
+        final Class<?> clss = instance.getClass();


Can we do final AbstractCheck checkInstance = (AbstractCheck) SiteUtil.getModuleInstance(moduleName); and do checkInstance.getClass() in called methods to reduce the number of parameters and casts?

It's not certain that it's always an AbstractCheck. For example filters.

stoyanK7 · 2023-08-29T12:24:18Z

Resolving comments right now, will move to ready for review when done.

stoyanK7 · 2023-08-29T15:23:03Z

Moving back to ready for review even though CI will fail. Moved quite a bit of stuff around.

stoyanK7 · 2023-08-29T15:24:33Z

- * Property {@code allowSamelineSingleParameterlessAnnotation} - Allow single parameterless
+ * Property {@code allowSamelineParameterizedAnnotation} - Allow one and only parameterized
 * annotation to be located on the same line as target element.
 * Type is {@code boolean}.
- * Default value is {@code true}.
+ * Default value is {@code false}.
 * </li>
 * <li>
- * Property {@code allowSamelineParameterizedAnnotation} - Allow one and only parameterized
+ * Property {@code allowSamelineSingleParameterlessAnnotation} - Allow single parameterless
 * annotation to be located on the same line as target element.
 * Type is {@code boolean}.
- * Default value is {@code false}.
+ * Default value is {@code true}.


The reason why the metadata file and the javadoc are changed is because of the order of the properties. Properties didn't have any order until now(or do they?) so it's impossible to match their order. The order now follows the order of the setters in the module class.

XDocs Order:

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

Line 1551 in 54649c9

final Set<String> result = new TreeSet<>();

XdocsJavaDocsTest took XDoc as it is (order and all) and made sure it was in the correct format as the Javadoc.

JavadocMetadataScraper just took what was in Javadoc and placed it into meta file in the same order as the Javadoc since it uses an array list.

checkstyle/src/main/java/com/puppycrawl/tools/checkstyle/meta/ModuleDetails.java

Line 30 in 54649c9

private final List<ModulePropertyDetails> properties = new ArrayList<>();

It seems to me, javadoc should be ordered just like xdoc, which is alphabetical order by TreeSet.

Looks like your issue is at:

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

Line 401 in 5050ae1

final Set<String> result = new LinkedHashSet<>(properties);

You switch over from the TreeSet to a LinkedHashSet. You shouldn't use hash sorting as different VMs can possibly create different hashes. Even if we were guaranteed hashes, we want alpha ordering here.

You're right, getProperties uses TreeSet and then I used LinkedHashSet in this subsequent method for some reason. Changed to TreeSet.

But still. Until now, xdoc does not seem to follow alphabetical order. The only ordering asserted is whether tokens/javadocTokens is last:

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

Lines 869 to 938 in 54649c9

private static void validatePropertySectionProperties(String fileName, String sectionName,

Node table, Object instance, Set<String> properties) throws Exception {

boolean skip = true;

boolean didJavadocTokens = false;

boolean didTokens = false;

for (Node row : XmlUtil.getChildrenElements(table)) {

final List<Node> columns = new ArrayList<>(XmlUtil.getChildrenElements(row));

assertWithMessage(fileName + " section '" + sectionName

+ "' should have the requested columns")

.that(columns)

.hasSize(5);

if (skip) {

assertWithMessage(fileName + " section '" + sectionName

+ "' should have the specific title")

.that(columns.get(0).getTextContent())

.isEqualTo("name");

assertWithMessage(fileName + " section '" + sectionName

+ "' should have the specific title")

.that(columns.get(1).getTextContent())

.isEqualTo("description");

assertWithMessage(fileName + " section '" + sectionName

+ "' should have the specific title")

.that(columns.get(2).getTextContent())

.isEqualTo("type");

assertWithMessage(fileName + " section '" + sectionName

+ "' should have the specific title")

.that(columns.get(3).getTextContent())

.isEqualTo("default value");

assertWithMessage(fileName + " section '" + sectionName

+ "' should have the specific title")

.that(columns.get(4).getTextContent())

.isEqualTo("since");

skip = false;

continue;

}

assertWithMessage(fileName + " section '" + sectionName

+ "' should have token properties last")

.that(didTokens)

.isFalse();

final String propertyName = columns.get(0).getTextContent();

assertWithMessage(fileName + " section '" + sectionName

+ "' should not contain the property: " + propertyName)

.that(properties.remove(propertyName))

.isTrue();

if ("tokens".equals(propertyName)) {

final AbstractCheck check = (AbstractCheck) instance;

validatePropertySectionPropertyTokens(fileName, sectionName, check, columns);

didTokens = true;

}

else if ("javadocTokens".equals(propertyName)) {

final AbstractJavadocCheck check = (AbstractJavadocCheck) instance;

validatePropertySectionPropertyJavadocTokens(fileName, sectionName, check, columns);

didJavadocTokens = true;

}

else {

assertWithMessage(fileName + " section '" + sectionName

+ "' should have javadoc token properties next to last, before tokens")

.that(didJavadocTokens)

.isFalse();

validatePropertySectionPropertyEx(fileName, sectionName, instance, columns,

propertyName);

}

Take https://checkstyle.org/checks/annotation/annotationusestyle.html for example

elementStyle

closingParens

trailingArrayComma

The only ordering asserted is whether tokens/javadocTokens is last

This was intentional as that was the ordering of tokens when I built the test.

Take https://checkstyle.org/checks/annotation/annotationusestyle.html for example
elementStyle
closingParens
trailingArrayComma

I could have sworn we ordered properties. It looks like we just ensure all properties are listed, and the order in XDoc is the order they should be in, regardless if it is alpha or random.

I recommend creating a new issue for this and this being done first. It won't be easy to review if there is a bunch of property changes like this.

I recommend creating a new issue for this and this being done first. It won't be easy to review if there is a bunch of property changes like this.

I second this.

Can we implement alphabet ordering now, but do not migrate numerous checks for now. So we can do:
1)migration to macroses in smaller batches

2)make massive updates for ordering first and then separate PRs wave to use macroses.

We will see later on what is less on effort.

Opened an issue at #13666 and PR at #13667

stoyanK7 · 2023-08-29T15:26:31Z


    /**
-     * Sets whether we should apply the check to public members.
+     * Setter to control whether we should apply the check to public members.


Changed so we can match what's already in the documentation:
https://checkstyle.org/checks/naming/constantname.html#Properties

name description

applyToPublic Controls whether to apply the check to public member.

applyToProtected Controls whether to apply the check to protected member.

This was a limitation since Checkstyle does not support multi-file.

romani

items:

romani

items:

nrmancuso

Few more:

nrmancuso

Few more:

romani · 2023-08-30T13:25:08Z

+     * @param moduleName the module name.
+     */
+    public static void setModuleName(String moduleName) {
+        ClassAndPropertiesSettersJavadocScraper.moduleName = moduleName;


we can get name of class from file name, do we need this setter ?
May be it is same question as making Module as stateless that I raised above.

stoyanK7 · 2023-08-30T18:21:14Z

+        private static boolean isInCodeLiteral;
+        /** Variable that tells if we are in an HTML_ELEMENT token. */
+        private static boolean isInHtmlElement;
+        /** Variable that tells if we are in an HREF attribute. */
+        private static boolean isInHrefAttribute;


IDEA inspections:
https://app.circleci.com/pipelines/github/checkstyle/checkstyle/20544/workflows/29b03a15-8b8d-485a-bd8a-2bd63d1b2aa4/jobs/373704/artifacts

Static field <code>isInHrefAttribute</code> may not be initialized during class initialization #loc ...

Initialize it with false and checkstyle complains it is initialized to default value. Place it back in method and method becomes too complex. I am stuck with this one.

https://sonarcloud.io/project/issues?resolved=false&types=CODE_SMELL&sinceLeakPeriod=true&pullRequest=13407&id=org.checkstyle%3Acheckstyle
Regarding the Sonarqube code smells - the first 2 ones with the brackets are not applicable because line length would become more than 100

Regarding the 2 methods that are too complex getDescriptionFromJavadoc and getDefaultValue, I don't think there's anything to gain from splitting them.

for Inspecton: put @noisnpection StaticVariableInitialization look at more example in our code on how to do this.

For Sonar: I agree, we can merge with violations and I suppress them in Sonar UI after merge.

romani

I relaunched failed CircleCIs

last items:

romani

item

nrmancuso

Last from me:

romani

Ok to merge.

Thanks a lot for your hard work on this

nrmancuso

This is good for now, let's merge this and improve later.

stoyanK7 marked this pull request as ready for review August 24, 2023 17:59

stoyanK7 mentioned this pull request Aug 25, 2023

Reuse in metadata module scrapper all methods to get data from javadoc and reflection only #13626

Closed

nrmancuso suggested changes Aug 26, 2023

View reviewed changes

nrmancuso self-assigned this Aug 26, 2023

romani requested changes Aug 26, 2023

View reviewed changes

nrmancuso suggested changes Aug 27, 2023

View reviewed changes

stoyanK7 marked this pull request as draft August 28, 2023 18:06

stoyanK7 marked this pull request as ready for review August 29, 2023 15:22

stoyanK7 commented Aug 29, 2023

View reviewed changes

stoyanK7 mentioned this pull request Aug 30, 2023

Assert that properties are ordered alphabetically in xdoc #13666

Closed

romani requested changes Aug 30, 2023

View reviewed changes

Comment thread src/main/java/com/puppycrawl/tools/checkstyle/site/PropertiesMacro.java Outdated

Comment thread src/main/java/com/puppycrawl/tools/checkstyle/site/SiteUtil.java Outdated

romani reviewed Aug 30, 2023

View reviewed changes

Comment thread src/main/java/com/puppycrawl/tools/checkstyle/site/PropertiesMacro.java Outdated

romani requested changes Aug 30, 2023

View reviewed changes

Comment thread src/main/java/com/puppycrawl/tools/checkstyle/site/PropertiesMacro.java Outdated

Comment thread src/main/java/com/puppycrawl/tools/checkstyle/site/PropertiesMacro.java Outdated

nrmancuso suggested changes Aug 30, 2023

View reviewed changes

Comment thread src/main/java/com/puppycrawl/tools/checkstyle/site/SiteUtil.java Outdated

Comment thread src/main/java/com/puppycrawl/tools/checkstyle/site/SiteUtil.java Outdated

Comment thread src/main/java/com/puppycrawl/tools/checkstyle/site/SiteUtil.java Outdated

romani reviewed Aug 30, 2023

View reviewed changes

stoyanK7 commented Aug 30, 2023

View reviewed changes

romani requested changes Aug 31, 2023

View reviewed changes

Comment thread src/main/java/com/puppycrawl/tools/checkstyle/site/PropertiesMacro.java Outdated

Comment thread src/main/java/com/puppycrawl/tools/checkstyle/site/PropertiesMacro.java Outdated

romani requested changes Aug 31, 2023

View reviewed changes

Comment thread src/main/java/com/puppycrawl/tools/checkstyle/site/PropertiesMacro.java Outdated

romani reviewed Aug 31, 2023

View reviewed changes

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

nrmancuso suggested changes Aug 31, 2023

View reviewed changes

nrmancuso reviewed Aug 31, 2023

View reviewed changes

Comment thread src/main/java/com/puppycrawl/tools/checkstyle/site/ClassAndPropertiesSettersJavadocScraper.java Outdated

nrmancuso mentioned this pull request Aug 31, 2023

Moved utility methods from CheckUtil to MissingJavadocMethodCheck #13607

Merged

rnveach mentioned this pull request Aug 31, 2023

Move CheckUtil#isGetterMethod and CheckUtil#isSetterMethod to MissingJavadocMethodCheck #13596

Closed

Issue #13399: Implement module properties macro

d093a4f

romani mentioned this pull request Aug 31, 2023

Improve design of PropertiesMacros #13673

Closed

romani approved these changes Aug 31, 2023

View reviewed changes

nrmancuso approved these changes Aug 31, 2023

View reviewed changes

nrmancuso merged commit 97f5591 into checkstyle:master Aug 31, 2023

nrmancuso mentioned this pull request Aug 31, 2023

Implement module properties macro #13399

Closed

stoyanK7 mentioned this pull request Sep 1, 2023

Implement module java parser #13177

Closed

		result = '"' + value.toString().replace("\n", "\\n").replace("\t", "\\t")
		.replace("\r", "\\r").replace("\f", "\\f") + '"';

		final Object instance = SiteUtil.getModuleInstance(moduleName);
		final Class<?> clss = instance.getClass();

	private static void validatePropertySectionProperties(String fileName, String sectionName,
	Node table, Object instance, Set<String> properties) throws Exception {
	boolean skip = true;
	boolean didJavadocTokens = false;
	boolean didTokens = false;

	for (Node row : XmlUtil.getChildrenElements(table)) {
	final List<Node> columns = new ArrayList<>(XmlUtil.getChildrenElements(row));

	assertWithMessage(fileName + " section '" + sectionName
	+ "' should have the requested columns")
	.that(columns)
	.hasSize(5);

	if (skip) {
	assertWithMessage(fileName + " section '" + sectionName
	+ "' should have the specific title")
	.that(columns.get(0).getTextContent())
	.isEqualTo("name");
	assertWithMessage(fileName + " section '" + sectionName
	+ "' should have the specific title")
	.that(columns.get(1).getTextContent())
	.isEqualTo("description");
	assertWithMessage(fileName + " section '" + sectionName
	+ "' should have the specific title")
	.that(columns.get(2).getTextContent())
	.isEqualTo("type");
	assertWithMessage(fileName + " section '" + sectionName
	+ "' should have the specific title")
	.that(columns.get(3).getTextContent())
	.isEqualTo("default value");
	assertWithMessage(fileName + " section '" + sectionName
	+ "' should have the specific title")
	.that(columns.get(4).getTextContent())
	.isEqualTo("since");

	skip = false;
	continue;
	}

	assertWithMessage(fileName + " section '" + sectionName
	+ "' should have token properties last")
	.that(didTokens)
	.isFalse();

	final String propertyName = columns.get(0).getTextContent();
	assertWithMessage(fileName + " section '" + sectionName
	+ "' should not contain the property: " + propertyName)
	.that(properties.remove(propertyName))
	.isTrue();

	if ("tokens".equals(propertyName)) {
	final AbstractCheck check = (AbstractCheck) instance;
	validatePropertySectionPropertyTokens(fileName, sectionName, check, columns);
	didTokens = true;
	}
	else if ("javadocTokens".equals(propertyName)) {
	final AbstractJavadocCheck check = (AbstractJavadocCheck) instance;
	validatePropertySectionPropertyJavadocTokens(fileName, sectionName, check, columns);
	didJavadocTokens = true;
	}
	else {
	assertWithMessage(fileName + " section '" + sectionName
	+ "' should have javadoc token properties next to last, before tokens")
	.that(didJavadocTokens)
	.isFalse();

	validatePropertySectionPropertyEx(fileName, sectionName, instance, columns,
	propertyName);
	}

name	description
applyToPublic	Controls whether to apply the check to public member.
applyToProtected	Controls whether to apply the check to protected member.

Uh oh!

Conversation

stoyanK7 commented Jul 18, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

romani commented Aug 8, 2023

Uh oh!

nrmancuso commented Aug 17, 2023

Uh oh!

romani commented Aug 26, 2023

Uh oh!

nrmancuso left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

romani left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nrmancuso left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nrmancuso Aug 27, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

stoyanK7 commented Aug 29, 2023

Uh oh!

stoyanK7 commented Aug 29, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rnveach Aug 29, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rnveach Aug 29, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rnveach Aug 29, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

stoyanK7 commented Jul 18, 2023 •

edited

Loading

nrmancuso left a comment •

edited

Loading

nrmancuso Aug 27, 2023 •

edited

Loading

rnveach Aug 29, 2023 •

edited

Loading

rnveach Aug 29, 2023 •

edited

Loading

rnveach Aug 29, 2023 •

edited

Loading

romani Aug 30, 2023 •

edited

Loading

romani left a comment •

edited

Loading