Issue #13626: Reuse in metadata module scrapper all methods to get data from javadoc and reflection only by stoyanK7 · Pull Request #13670 · checkstyle/checkstyle

stoyanK7 · 2023-08-30T18:42:47Z

Resolves #13626

nrmancuso · 2023-08-31T20:58:11Z

@stoyanK7 let's rebase and prepare for review :)

romani

items:

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

romani

Items

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

romani

Items

...resources/com/puppycrawl/tools/checkstyle/meta/checks/annotation/AnnotationLocationCheck.xml

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

stoyanK7 · 2023-09-04T17:16:36Z

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

+import javax.xml.parsers.ParserConfigurationException;
+import javax.xml.transform.TransformerException;
+
+import org.apache.maven.doxia.macro.MacroExecutionException;


It isn't ideal that the metadata module uses the site module. I think it's better if the opposite is the case - the site module uses the metadata module. Shall we deal with this in a follow-up issue?

We will refactor later on to move methods around to make it better design wise.
For now let's focus on functionality to unblock ability to remove properties from javadoc

Let's collect ideas at #13699 to improve in separate PRs

romani · 2023-09-04T17:58:43Z

I think PR is ready to not have commit for disablement of CIs, please remove.
Let's finalize initial version of metadata scrapper from javadoc only.

Please rephrase commit message, as you do not rewrite but you change a way we get data and we will do this very gradually.

…ta from javadoc and reflection only

romani · 2023-09-04T20:44:21Z

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

+     * @param propertyJavadoc property javadoc.
+     * @return property description.
+     */
+    private static String getPropertyDescription(String property, DetailNode propertyJavadoc) {


I am not sure why this method is specific for metadata.
I think there should single method that does it for xdoc and meta and meta generation just reuse it
Same for getPropertyDefaultValue

If agree please create separate issue to improve in separate PR

If you agree add it to #13699

I think there should be separate methods for xdoc and metadata. Or at least single method with different output depending on who/what is calling it.

Metadata gets text as-is and doesn't care about any processing.

Xdoc requires to

convert {@code something} to <code>something</code>

Convert links to relative ones- https://checkstyle.org/checks/block/xxx.html -> ../block/xx.html, for example

But still data creation should be single method and xdoc just do only such required formatting. If you agree just put this to separate issue that we have for design improvement as item.

romani

Ok to merge to unblock massive removal of javadoc content.
We will improve in separate PRs

romani · 2023-09-04T20:47:14Z

@stoyanK7 , please make CI green.

nrmancuso

Items:

nrmancuso · 2023-09-05T13:24:27Z

pom.xml

                    <exclude>
                      com.puppycrawl.tools.checkstyle.api.AutomaticBean*
                    </exclude>
+                    <exclude>com.puppycrawl.tools.checkstyle.meta.MetadataGeneratorUtil</exclude>


@romani should we create an issue for coverage in this class and revisit later?

This is jacoco coverage.
The only time it makes sense if we ever move all functionality to separate project and we remote xdoc files from git completely, as we will never see how content looks like before html generations.
Before this time, all such methods should not have to work on all possible cases, code should serve our current cases of Checks and xdoc/meta. So coverage has no much meaning.
Do we need such issue ? As we do not even has plan to move it out

If you agree, please copy my comment or part of it to description at #13426 as requirements

nrmancuso · 2023-09-05T13:27:02Z

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

+    /** No-argument constructor. */
+    public ModuleDetails() {
+        // empty constructor
+    }


Why do we need this?

Compile error https://stackoverflow.com/a/5286532/1015848
We still have old code of metadata that use default ctor.

We are not rewriting metadata, we migrate it to use new method to get data that are based on reflection and javadoc

There is usage of it in JavadocMetadataScraper

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

Line 192 in dbcbd67

moduleDetails = new ModuleDetails();

nrmancuso · 2023-09-05T13:27:33Z

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

+    /** No-argument constructor. */
+    public ModulePropertyDetails() {
+        // empty constructor
+    }


Same as above, why do we need this?

Same as above

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

Line 295 in dbcbd67

final ModulePropertyDetails modulePropertyDetails = new ModulePropertyDetails();

nrmancuso · 2023-09-05T13:30:08Z

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

+                Lists.reverse(Arrays.asList(detailNode.getChildren())).forEach(stack::push);
+
+                final String childText = detailNode.getText();
+                // Regular expression for detecting ANTLR tokens(for e.g. CLASS_DEF).


Unless I am missing something, we are not doing anything with ANTLR tokens, ANTLR is an internal detail of parser/AST implementation. Outside of these usages, these just become JavadocTokenTypes.

Can we just use

checkstyle/src/main/java/com/puppycrawl/tools/checkstyle/utils/TokenUtil.java

Line 96 in 54616f6

public static Map<String, Integer> nameToValueMapFromPublicIntFields(Class<?> cls) {

to get a list of actual tokens and check that?

nrmancuso · 2023-09-05T13:37:45Z

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

+            final Path templatePath = SiteUtil.getTemplatePath(
+                SiteUtil.removeCheckSuffix(SiteUtil.getModuleName(file))


We are getting crazy with utility method usage and creation. Why can't we do a simple replace with Check as target?

nrmancuso · 2023-09-05T13:47:20Z

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

+     * @param moduleName module name.
+     * @return module type.
+     */
+    private static ModuleType getModuleType(String moduleName) {


Can we reuse/repurpose https://github.com/nrmancuso/checkstyle/blob/54616f662130aa0ca5afe45f72d36cdefb78a781/src/main/java/com/puppycrawl/tools/checkstyle/meta/JavadocMetadataScraper.java#L572 ? This seems like a lot of duplication to me.

You mean make the one in JavadocMetadataScraper public?

I have no strong feelings either way, but we should consider some way to avoid massive copy paste

nrmancuso · 2023-09-06T02:38:12Z

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

+
+                final String childText = detailNode.getText();
+                // Regular expression for detecting ANTLR tokens(for e.g. CLASS_DEF).
+                final Pattern tokenTextPattern = Pattern.compile("([A-Z_]{2,})+");


Why compile this pattern repeatedly? Can we make this a field? Even better, can we just use

checkstyle/src/main/java/com/puppycrawl/tools/checkstyle/utils/TokenUtil.java

Line 96 in 54616f6

public static Map<String, Integer> nameToValueMapFromPublicIntFields(Class<?> cls) {

to get the actual tokens?

nrmancuso · 2023-09-06T02:52:08Z

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

+        final Set<Path> templatesPaths = SiteUtil.getXdocsTemplatesFilePaths();
+        for (Path templatePath: templatesPaths) {
+            final String content = getFileContents(templatePath);
+            final String propertiesMacroDefinition = "<macro name=\"properties\"";
+            if (content.contains(propertiesMacroDefinition)) {
+                result.add(templatePath);
+            }
+        }
+        return result;


psuedocode:

return SiteUtil.getXdocsTemplatesFilePaths().stream() .map(SiteUtil::getFileContents) .filter(c -> c.contains(p)) .collect(Collectors.toList());

nrmancuso · 2023-09-06T02:56:52Z

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

+    }
+
+    /**
+     * Get validation type for the given property.


What is a validation type?

Actual type is int[] but business logic wise it is int values from "enum"-like class of tokens. So this helps plugins to know how to validate content. Ideally it should be hinted by source code but we never end up doing it explicitly.

checkstyle/src/main/resources/com/puppycrawl/tools/checkstyle/meta/checks/annotation/AnnotationLocationCheck.xml

Line 59 in f58e45e

validation-type="tokenSet">

Only for tokens https://github.com/search?q=repo%3Acheckstyle%2Fcheckstyle+validation-type&type=code&p=1

nrmancuso · 2023-09-06T02:58:16Z

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

+            Set<String> properties, Map<String, DetailNode> javadocs,
+            String className, Object instance)
+            throws MacroExecutionException {
+        final List<ModulePropertyDetails> result = new ArrayList<>(properties.size());


result -> modulePropertyDetails

rnveach · 2023-09-07T17:50:26Z

@stoyanK7 Since we are re-writing meta, did you verify if any of your changes will break eclipse-cs or sonar?

romani · 2023-09-09T03:36:51Z

We are not rewriting, we are using different approach to get data for metadata files that we generate. No changes in files, it means it works as before.

romani · 2024-03-16T13:50:55Z

very sad that me failed to merge this, issue is still open, I hope we can come back to it.

rnveach added the blocked label Aug 31, 2023

romani requested changes Sep 2, 2023

View reviewed changes

src/main/java/com/puppycrawl/tools/checkstyle/meta/MetadataGeneratorUtil.java Outdated Show resolved Hide resolved

src/main/java/com/puppycrawl/tools/checkstyle/meta/MetadataGeneratorUtil.java Outdated Show resolved Hide resolved

romani removed the blocked label Sep 2, 2023

romani requested changes Sep 3, 2023

View reviewed changes

romani marked this pull request as ready for review September 4, 2023 16:50

romani requested changes Sep 4, 2023

View reviewed changes

...resources/com/puppycrawl/tools/checkstyle/meta/checks/annotation/AnnotationLocationCheck.xml Outdated Show resolved Hide resolved

stoyanK7 commented Sep 4, 2023

View reviewed changes

src/main/java/com/puppycrawl/tools/checkstyle/meta/MetadataGeneratorUtil.java Show resolved Hide resolved

stoyanK7 commented Sep 4, 2023

View reviewed changes

Issue #13626: Reuse in metadata module scrapper all methods to get da…

851a44d

…ta from javadoc and reflection only

stoyanK7 changed the title ~~Issue #13626: Rewrite metadata module~~ Issue #13626: Reuse in metadata module scrapper all methods to get data from javadoc and reflection only Sep 4, 2023

romani reviewed Sep 4, 2023

View reviewed changes

romani approved these changes Sep 4, 2023

View reviewed changes

romani assigned nrmancuso Sep 4, 2023

romani requested a review from nrmancuso September 4, 2023 20:46

nrmancuso suggested changes Sep 6, 2023

View reviewed changes

stoyanK7 mentioned this pull request Sep 9, 2023

Decide on how to process validation-type #13722

Open

romani mentioned this pull request Nov 30, 2023

Issue #13693: migrate header modules to use property macro #14048

Merged

nrmancuso removed their assignment Mar 5, 2024

romani mentioned this pull request Mar 16, 2024

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

Closed

romani added the abandoned label Mar 16, 2024

romani closed this Mar 16, 2024

SteLeo1602 mentioned this pull request Aug 23, 2025

Issue #13626: Reused meta and xdoc functionality to gut annotation module javadocs #17675

Merged

		final Path templatePath = SiteUtil.getTemplatePath(
		SiteUtil.removeCheckSuffix(SiteUtil.getModuleName(file))

Uh oh!

Conversation

stoyanK7 commented Aug 30, 2023

Uh oh!

nrmancuso commented Aug 31, 2023

Uh oh!

romani left a comment

Choose a reason for hiding this comment

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!

romani left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

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!

romani commented Sep 4, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

romani Sep 4, 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!

romani left a comment

Choose a reason for hiding this comment

Uh oh!

romani commented Sep 4, 2023

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!

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

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

romani commented Sep 4, 2023 •

edited

Loading

romani Sep 4, 2023 •

edited

Loading

romani Sep 6, 2023 •

edited

Loading