Issue #13422: Implement module violation messages macro by stoyanK7 · Pull Request #13423 · checkstyle/checkstyle

stoyanK7 · 2023-07-24T12:37:00Z

Resolves #13422

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

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

nrmancuso

Items, some are contingent on response at #13423 (comment) :

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

nrmancuso · 2023-07-26T19:15:04Z

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

+     */
+    private static Set<String> getMessageKeys(Set<Field> messageKeyFields, Object instance)
+            throws MacroExecutionException {
+        final Set<String> messageKeys = new TreeSet<>();


Why TreeSet?

We want to preserve the order of the keys. Using HashSet messed up the order and failed XdocsJavadocsTest. That's what's being done in XdocsPagesTest

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

Lines 1381 to 1389 in 0c2ab53

final Set<Field> fields = CheckUtil.getCheckMessages(clss, true);

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

for (Field field : fields) {

// below is required for package/private classes

field.trySetAccessible();

list.add(field.get(null).toString());

}

Hm, why are we forced to use Set at all? Do we want to maintain insertion order? If so, we should use LinkedHashSet.

I got rid of the Set and just used a List.

@rnveach could you please tell if using a Set here is a must? Could there be some duplicates or?

No duplicates are allowed in messages

Set, will hide duplicates to make illusion in html.
Better to use List to simply print as is.

The only extra benefit from fancier collection I see, is ordering.
It will be good to order alphabetically in html.
But we can do this later on, after transition to templates is done.

It will be good to order alphabetically in html.

It is already done in ViolationMessagesMacro::createListOfMessages

Collections.sort(messageKeys);

You can't use a collection that is based on hashcode. It is JVM specific and different runs can generate different hashes and orders. We can't use insertion order here because reflection is not guaranteed an order and we want to ensure our documentation is consistent in order. It will be bad if one run says A,B,C and then another run says C,A,B. This is why we originally used TreeSet, to say order is alphabetic.

I recommend keeping this a copy/paste and just create new issues to fix anything.

@rnveach awesome explanation, thanks a lot. @stoyanK7 please restore copy pasta and add a comment about why we use treeset

@nrmancuso Done at #13505.

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

nrmancuso · 2023-07-31T14:28:50Z

I would like to make sure we have response from @rnveach at #13423 (comment) before we merge.

romani

Items

config/suppressions.xml

config/spotbugs-exclude.xml

config/intellij-idea-inspections.xml

pom.xml

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

src/xdocs/checks/annotation/annotationlocation.xml

romani

Items

config/intellij-idea-inspections.xml

romani

Items

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

rnveach · 2023-08-02T02:02:22Z

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

+        final Object instance = SiteUtil.getModuleInstance(checkName);
+        final Class<?> clss = instance.getClass();
+        final List<String> messageKeys = SiteUtil.getCheckMessageKeys(clss, instance);
+        createListOfMessages((XdocSink) sink, clss, messageKeys);


So what is the plan in terms of disabling javadoc. Are we not going to have these type of generation in meta and have our macros get the data from meta? To me this makes the most sense, instead of meta having another custom implementation.

This implementation will substitute getting data from metadata.
For now validation javadoc vs xdoc help to test this implementation until it gets its maturity.
Such "another implementation" will me main implementation and source of truth soon.

rnveach · 2023-08-02T02:04:02Z

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

+ * A macro that inserts a list of the violation messages.
+ */
+@Component(role = Macro.class, hint = "violation-messages")
+public class ViolationMessagesMacro extends AbstractMacro {


So we are not going to disable the violation portion of the xdocspages test for classes we have converted to use macros to show it isn't needed anymore?

Not for now, but as we convert all modules to templates, and all continue to match, we can disable it. May be we will make some hack to allow migration suppression for such validation.

rnveach · 2023-08-02T02:10:36Z

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

+/**
+ * Utility class for site generation.
+ */
+public final class SiteUtil {


If we are leaving this in production, then why don't we remove the test code and have it use this instead?

This code is still planned to be moved to separate project. I am ok with such transitional copy paste of utils.

romani

Ok to merge

stoyanK7 commented Jul 24, 2023

View reviewed changes

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

stoyanK7 marked this pull request as ready for review July 26, 2023 16:36

stoyanK7 commented Jul 26, 2023

View reviewed changes

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

nrmancuso suggested changes Jul 26, 2023

View reviewed changes

romani assigned nrmancuso Jul 29, 2023

nrmancuso approved these changes Jul 31, 2023

View reviewed changes

nrmancuso assigned rnveach and unassigned nrmancuso Jul 31, 2023

stoyanK7 mentioned this pull request Jul 31, 2023

Refactor ViolationMessagesMacro::constructMessageKeyUrl to use UrlEncoder #13490

Closed

romani requested a review from rnveach July 31, 2023 20:34

romani requested changes Aug 1, 2023

View reviewed changes

config/intellij-idea-inspections.xml Outdated Show resolved Hide resolved

stoyanK7 mentioned this pull request Aug 1, 2023

Create templates from xdocs #13498

Closed

17 tasks

romani requested changes Aug 1, 2023

View reviewed changes

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

stoyanK7 mentioned this pull request Aug 1, 2023

Fix abstraction failure in SiteUtil::getCheckMessageKeys #13500

Open

Issue #13422: Implement module violation messages macro

b5bf5f5

rnveach reviewed Aug 2, 2023

View reviewed changes

romani approved these changes Aug 2, 2023

View reviewed changes

rnveach assigned romani and unassigned rnveach Aug 3, 2023

romani merged commit ec5cd7e into checkstyle:master Aug 3, 2023

stoyanK7 deleted the 13422 branch August 3, 2023 10:34

stoyanK7 mentioned this pull request Aug 3, 2023

Pull #13505: Restore usage of TreeSet #13505

Merged

This was referenced Aug 25, 2023

Use violation messages macro in all templates #13627

Closed

Decide on how to handle edge cases for @MessageKey #13160

Closed

stoyanK7 mentioned this pull request Sep 1, 2023

Implement module java parser #13177

Closed

	final Set<Field> fields = CheckUtil.getCheckMessages(clss, true);
	final Set<String> list = new TreeSet<>();

	for (Field field : fields) {
	// below is required for package/private classes
	field.trySetAccessible();

	list.add(field.get(null).toString());
	}

Uh oh!

Conversation

stoyanK7 commented Jul 24, 2023

Uh oh!

Uh oh!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nrmancuso Jul 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!

rnveach Aug 2, 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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nrmancuso commented Jul 31, 2023

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!

romani left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

romani left a comment

Choose a reason for hiding this comment

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!

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!

Reviewers

Assignees

Labels

Projects

nrmancuso left a comment •

edited

Loading

nrmancuso Jul 29, 2023 •

edited

Loading

rnveach Aug 2, 2023 •

edited

Loading