Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

New Javadoc module for checking preceding <p> tag for block level tags #15458

Closed
Zopsss opened this issue Aug 6, 2024 · 7 comments
Closed

New Javadoc module for checking preceding <p> tag for block level tags #15458

Zopsss opened this issue Aug 6, 2024 · 7 comments
Assignees
@romani
Labels
google style

Comments

@Zopsss
Copy link
Member

Zopsss commented Aug 6, 2024 •
edited by romani
Loading

From https://github.com/checkstyle/checkstyle/pull/14901/files#r1622428372

Investigated at :
#15011 (comment) (there are screenshots of javadoc with extra P tag)

7.1.2 Paragraphs rule of google java style guide states that there should not be preceding <p> tag for block tags:

HTML tags for other block-level elements, such as <ul> or <table>, are not preceded with <p>.

Whole rule:

7.1.2 Paragraphs
One blank line—that is, a line containing only the aligned leading asterisk (*)—appears between paragraphs, and before the group of block tags if present. Each paragraph except the first has <p> immediately before the first word, with no space after it. HTML tags for other block-level elements, such as <ul> or <table>, are not preceded with <p>.

We currently have no support to check this, there's also a message explaining this gap in our config at coverage table:

We don't have support to check for the extra preceding p tag before block tag; there is known issue to cover it fully: #15011

So we need a new Check to ensure we follow this rule. It will be javadoc check, it can named something like: JavadocInvalidParagraphPosition ( suggestions are welcomed )

How it should work?

This is a list of block-level tags in HTML:

address
article
aside
blockquote
canvas
dd
div
dl
dt
fieldset
figcaption
figure
footer
form
h1
h2
h3
h4
h5
h6
header
hr
li
main
nav
noscript
ol
p
pre
section
table
tfoot
ul
video

source w3 schools: https://www.w3schools.com/html/html_blocks.asp

We should check if <p> is not preceding any of such block level tags which are used in javadoc.

Example:

/**
 * <p><h1>Testing...</h1> // warn
 */
public class InvalidParagraphTagPosition {
    /**
     * <p><ul> // warn
     *  <p><li>1</li> // warn
     *  <p><li>1</li></p> // warn
     *  <p>
     *  <li>1</li> // warn
     * </ul>
     * </p>
     * </p>
     */
    public void foo() {}

    /**
     * <p>
     * <table> // warn
     * </table>
     */
    public void fooo() {}
}

Config:

<?xml version="1.0"?>
<!DOCTYPE module PUBLIC
          "-//Checkstyle//DTD Checkstyle Configuration 1.3//EN"
          "https://checkstyle.org/dtds/configuration_1_3.dtd">

<module name="Checker">
  <module name="TreeWalker">
    <module name="JavadocInvalidParagraphPosition" />
  </module>
</module>

Output:

$ java -jar .\checkstyle-10.17.0-all.jar -c .\config.xml  .\InvalidParagraphTagPosition.java
Starting audit...
[ERROR] .\InvalidParagraphTagPosition.java:2:7:  Block level tag "<h1>" should not be preceded with "<p>"
[ERROR] .\InvalidParagraphTagPosition.java:6:11:  Block level tag "<ul>" should not be preceded with "<p>"
[ERROR] .\InvalidParagraphTagPosition.java:7:12:  Block level tag "<li>" should not be preceded with "<p>"
[ERROR] .\InvalidParagraphTagPosition.java:8:12:  Block level tag "<li>" should not be preceded with "<p>"
[ERROR] .\InvalidParagraphTagPosition.java:10:9:  Block level tag "<li>" should not be preceded with "<p>"
[ERROR] .\InvalidParagraphTagPosition.java:19:8:  Block level tag "<table>" should not be preceded with "<p>"
Audit done.
Checkstyle ends with 6 errors.

The warning is given for the block tag, explaining the tag is preceded by <p> which is not allowed.

Additional context

The coverage table website is generated by GitHub, generate site. We have not released new version so the live website does not contain explaining message, you won't be able to see it there.
@Zopsss Zopsss moved this to For Approval in Refine Google Style Guide Aug 6, 2024
@romani
Copy link
Member

romani commented Aug 9, 2024

First and foremost we should update https://checkstyle.org/checks/javadoc/javadocparagraph.html#JavadocParagraph to not demand (place violation) before block tags. Let's keep this issue for update to existing Check.
Please update issue description to focus only on JavadocParagraph.

We can extend this Check to report on extra P tags. This Check was created fot google style.

@rdiachenko
Copy link
Contributor

First and foremost we should update https://checkstyle.org/checks/javadoc/javadocparagraph.html#JavadocParagraph to not demand (place violation) before block tags.

I don't get it. The guide says that there should be a blank line before the group of block tags if present which is alredy covered by RequireEmptyLineBeforeBlockTagGroup. What do you mean by to not demand (place violation) before block tags?

We can extend this Check to report on extra P tags.

Agree, this part HTML tags for other block-level elements, such as <ul> or <table>, are not preceded with <p>. should be covered by extending JavadocParagraph check.

@rdiachenko rdiachenko assigned romani and unassigned rdiachenko Aug 10, 2024
@romani
Copy link
Member

romani commented Aug 10, 2024 •
edited
Loading

rule is:

7.1.2 Paragraphs
One blank line—that is, a line containing only the aligned leading asterisk (*)—appears between paragraphs, and before the group of block tags if present. Each paragraph except the first has <p> immediately before the first word, with no space after it. HTML tags for other block-level elements, such as <ul> or <table>, are not preceded with <p>.

I pretty sure this JavadocParagraph is simply demand <p> at all places. But we should have no violations if <ul> starts a paragraph.

* <p><ul> // warn Check should place violation 'extra P tag before block tag'

@romani
Copy link
Member

romani commented Aug 10, 2024 •
edited
Loading

we have bigger problem.

checkstyle/src/main/java/com/puppycrawl/tools/checkstyle/checks/javadoc/JavadocParagraphCheck.java

Lines 152 to 154 in 66ba088

else if (ast.getType() == JavadocTokenTypes.HTML_ELEMENT
&& JavadocUtil.getFirstChild(ast).getType() == JavadocTokenTypes.P_TAG_START) {
checkParagraphTag(ast);

this Check does validation only if P tag is present. But Google rule is not about valdiation of P tag, it demands P tag to be present after empty line * .
So we have two options: create new Check or update this Check to validate presence of P (or any other block tag) before empty line.

I vote to update JavadocParagraph to raise violations on 2 new cases:

/**
 * Some Javadoc.
 *
 * <ul>Some sontent.</ul>
 *
 * <b>Some content.</b>
 */
 // violation 2 lines above 'P tag or block-level HTML elements should start paragraph'

/**
 * Some Javadoc.
 *
 * <p><ul> some </ul>
 */
 // violation 2 lines above 'P tag should not preceed block-level HTML elements '

@romani romani mentioned this issue Aug 10, 2024
Closed
@rdiachenko
Copy link
Contributor

I vote to update JavadocParagraph to raise violations on 2 new cases:

Sounds good to me. I don't think we need a new check, let's update JavadocParagraph.

@romani
Copy link
Member

romani commented Aug 11, 2024

@Zopsss , please close this issue and create new issue on update of existing Check.

@Zopsss Zopsss mentioned this issue Aug 14, 2024
Closed
@Zopsss
Copy link
Member Author

Zopsss commented Aug 14, 2024 •
edited
Loading

@Zopsss , please close this issue and create new issue on update of existing Check.

done. #15503

@Zopsss Zopsss closed this as completed Aug 14, 2024
@github-project-automation github-project-automation bot moved this from For Approval to Done in Refine Google Style Guide Aug 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@romani romani

Labels
google style
Projects
No open projects
Refine Google Style Guide
Status: Done
Milestone
No milestone
Development

No branches or pull requests
3 participants
@romani @rdiachenko @Zopsss