Skip to content

Issue #17281: Add new DescriptionMacro#17278

Merged
romani merged 1 commit into
checkstyle:masterfrom
SteLeo1602:descriptionMacro
Jul 4, 2025
Merged

Issue #17281: Add new DescriptionMacro#17278
romani merged 1 commit into
checkstyle:masterfrom
SteLeo1602:descriptionMacro

Conversation

@SteLeo1602

@SteLeo1602 SteLeo1602 commented Jun 25, 2025

Copy link
Copy Markdown
Contributor

Issue: #17281

@Zopsss

Zopsss commented Jun 25, 2025

Copy link
Copy Markdown
Member

Where is the issue for this PR?

@SteLeo1602 SteLeo1602 changed the title GSOC25: Add new DescriptionMacro Issue #17281: Add new DescriptionMacro Jun 25, 2025
@SteLeo1602

Copy link
Copy Markdown
Contributor Author

Where is the issue for this PR?

There, fixed it

@romani

romani commented Jun 26, 2025

Copy link
Copy Markdown
Member

Issue should be in commit prefix

@SteLeo1602

Copy link
Copy Markdown
Contributor Author

Issue should be in commit prefix

Done

@SteLeo1602 SteLeo1602 force-pushed the descriptionMacro branch 4 times, most recently from b5fed51 to 914cd04 Compare July 1, 2025 22:44
@romani

romani commented Jul 1, 2025

Copy link
Copy Markdown
Member

GitHub, generate website

@romani romani left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Items

Comment thread src/main/java/com/puppycrawl/tools/checkstyle/site/DescriptionMacro.java Outdated
@romani

romani commented Jul 2, 2025

Copy link
Copy Markdown
Member

GitHub, generate website

@romani romani left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Items

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

@romani romani left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

item:

@Zopsss

Zopsss commented Jul 2, 2025

Copy link
Copy Markdown
Member

@SteLeo1602 I wanted to debug the new DescriptionMacro.java file to better understand the code, do you know any ways to debug it?

@SteLeo1602

Copy link
Copy Markdown
Contributor Author

@SteLeo1602 I wanted to debug the new DescriptionMacro.java file to better understand the code, do you know any ways to debug it?

Just debug launch the XdocsPagesTest.java and see how DescriptionMacro gets the description and prints it out.

@romani romani left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ite

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

@romani romani left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Items

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

@romani romani left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok to merge.

@Zopsss , please finalize review

@Zopsss Zopsss left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the delay, I was having some difficulties with my IntelliJ.

Comment on lines +163 to +175
else if (index > 1
&& (previousProcessedLine.contains("<pre")
|| !previousProcessedLine.startsWith(INDENT_LEVEL_8))) {

final String currentLineWithPreservedIndent = moduleDescriptionLinesSplit[index]
.substring(1);
processedLine = NEWLINE + currentLineWithPreservedIndent;
}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this else if statement only for handling code blocks? I debugged the code, and this else if statement was used only when we encountered code blocks.

If yes, then the currentLineWithPreservedIndent variable name doesn't make much sense. There is no indentation for code blocks:

<div class="wrapper"><pre class="prettyprint"><code class="language-java">
public @Nullable Long getStartTimeOrNull() { ... }
</code></pre></div>

currentLineWithPreservedIndent should be renamed to currentCodeBlockLine or something similar, ask AI for better naming, I'm bad at naming things :p

@SteLeo1602 SteLeo1602 Jul 3, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This else if statement is for any block that is under the <pre></pre> tags which work to preserve the indentation. This might be mostly for code blocks, but not always, as I've only implemented this macro for 2 checks. So far, in this check example, there is only 1 line of code with no indent which is just an occasion, but for more complex code examples, there can be different levels of indentation just like in the actual java code which has to be accounted and not hard-given the identical indentation like for the rest of blocks.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I checked all module's description and mostly all of them had <pre> tag followed by <code> tag, there were <pre> tags without <code> but they weren't in module's description, at least I couldn't find such case.

Anyways, this conversation is Resolved 👍🏻

Comment on lines +118 to +128
/**
* Gets the start index of the parent subsection in module's JavaDoc.
*
* @param moduleJavadoc javadoc of module.
* @return start index of parent subsection.
*/
private static int getParentSectionStartIndex(DetailNode moduleJavadoc) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This method is used when the module doesn't have any properties, so we find the following part in module's javadoc:

 * <p>
 * Parent is {@code com.puppycrawl.tools.checkstyle.TreeWalker}
 * </p>

and return it's index.

The description is little confusing if you don't know what method does, so it should be simplified to something like:

Gets the starting index of the "Parent is" paragraph in module's javadoc.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@Zopsss

Zopsss commented Jul 3, 2025

Copy link
Copy Markdown
Member

spotbug CI failure is not related to the PR so it can be ignored

@SteLeo1602 SteLeo1602 force-pushed the descriptionMacro branch 2 times, most recently from fad4513 to 04c17cd Compare July 3, 2025 22:53
@SteLeo1602

Copy link
Copy Markdown
Contributor Author

GitHub, generate website

@Zopsss Zopsss assigned romani and unassigned Zopsss Jul 4, 2025
@romani romani merged commit a2364aa into checkstyle:master Jul 4, 2025
119 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants