Issue #15685: fixed JavadocParagraph to detect paragraph tag when they have their corresponding closing tag #15686

Zopsss · 2024-09-21T21:04:41Z

tried fixing the implementation of JavadocParagraph ~~might have introduced new false-negatives :p~~

I tried to make the Check work same way when the paragraph tag's corresponding closing tag is also present.

when I tried to run build command locally, I got so many errors for different files like checks, filters, etc for <p> tag. Most of them were for:

<p> tag should be placed immediately before the first word, with no space after.

@romani need your help here

Diff Regression config: https://gist.githubusercontent.com/Zopsss/013f379d4c821f2d624be56212a64575/raw/5f17ec9186d4421396429ab38cbac3e4663bf4eb/javadocparagraph_config.xml
Diff Regression projects: https://raw.githubusercontent.com/checkstyle/contribution/master/checkstyle-tester/projects-to-test-on-for-github-action.properties

Zopsss · 2024-09-24T10:07:24Z

I have set the allowNewlineParagraph to false in checkstyle-checks.xml and now there is no violation for

<p> tag should be placed immediately before the first word, with no space after.

though there are still following errors in the build failure:

<p> tag should be preceded with an empty line.
Redundant <p> tag.

Locally I tried to fix allowNewlineParagraph by doing something like this:

    /**
     * Tests whether the paragraph tag is misplaced.
     * Meaning that it is not immediate before the first word or another tag.
     *
     * @param tag html tag.
     * @return true, if the paragraph tag is misplaced.
     */
    private boolean isMisplacedParagraph(DetailNode tag) {
        boolean shouldGiveViolation;

        if (allowNewlineParagraph) {
            shouldGiveViolation = false;
        }
        else if (JavadocUtil.getFirstChild(tag).getType() == JavadocTokenTypes.PARAGRAPH) {
            final DetailNode paragraphToken = JavadocUtil.getFirstChild(tag);
            final DetailNode paragraphStartTagToken = JavadocUtil.getFirstChild(paragraphToken);
            final DetailNode nextSibling = JavadocUtil.getNextSibling(paragraphStartTagToken);
            shouldGiveViolation = nextSibling.getType() == JavadocTokenTypes.NEWLINE
                    || nextSibling.getType() == JavadocTokenTypes.EOF
                    || nextSibling.getText().startsWith(" ");
        }
        else {
            final DetailNode nextSibling = JavadocUtil.getNextSibling(tag);
            shouldGiveViolation = nextSibling.getType() == JavadocTokenTypes.NEWLINE
                    || nextSibling.getType() == JavadocTokenTypes.EOF
                    || nextSibling.getText().startsWith(" ");
        }

        return shouldGiveViolation;
    }

    /**
     * Determines whether or not the line with paragraph tag has previous empty line.
     *
     * @param tag html tag.
     */
    private void checkParagraphTag(DetailNode tag) {
        final DetailNode newLine = getNearestEmptyLine(tag);
        if (isFirstParagraph(tag)) {
            log(tag.getLineNumber(), MSG_REDUNDANT_PARAGRAPH);
        }
        else if (newLine == null || tag.getLineNumber() - newLine.getLineNumber() != 1) {
            log(tag.getLineNumber(), MSG_LINE_BEFORE);
        }
        if (isMisplacedParagraph(tag)) { // this line is changed.
            log(tag.getLineNumber(), MSG_MISPLACED_TAG);
        }
    }

With this there was no error of:

<p> tag should be placed immediately before the first word, with no space after.

and I also didn't needed to set allowNewlineParagraph to false. There were still plenty of other errors... which are other false-positives of the check.

romani · 2024-09-26T13:21:06Z

this Check code is very hard to read, lets refactor it a bit before this PR to proceeed.

please look at #15709 and try to finish it.
We need to split newline handling to make logic a bit more clear.

Zopsss · 2024-09-27T18:11:26Z

build is still failing, there are still lots of errors

romani · 2024-09-28T14:22:03Z

before we proceed on this, all other PRs and issues should be merged.
#15713 must be done before, as it will make inputs more realistic so we will make update more clearly.

romani · 2024-10-02T13:06:18Z

@Zopsss , please resolve conflicts in this PR to let me start review.
you can create new Inputs to avoid conflicts with pending PR for Block-tag.

Zopsss · 2024-10-02T18:20:47Z

we need to use for checkstyle-checks.xml
<property name="allowNewlineParagraph" value="false"/>

reverted this as we fixed this in one of my old PR.

Zopsss · 2024-10-02T18:25:55Z

local build was still failing, it was mostly giving errors for:

<p> tag should be preceded with an empty line.
Redundant <p> tag.

It was also giving this error:

<p> tag should be placed immediately before the first word, with no space after.

for cases like:

<p> testing: </p>
   ↑ whitespace

the above error is fine, it is violation of rule but other 2 errors are false-positive, they started appearing because we added support to check for open-close tag.

Should we keep these errors or not? If yes, then it would require changing lots of files.

romani · 2024-10-02T19:27:19Z

checkstyle/config/checkstyle-checks.xml

Line 720 in 37a777c

This means that allowNewlineParagraph= true

And it means that you have defect, as this property is to allow \n.

Zopsss · 2024-10-02T19:31:02Z

And it means that you have defect, as this property is to allow \n.

yes we allow it, so that check does not give violation on lines like:

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

Lines 28 to 31 in 37a777c

    
           /** 
        
            * <p> 
        
            * Checks the Javadoc paragraph. 
        
            * </p>

the default property is what we need for our style. We set it to false for google_checks.xml because it does not follow such rules.

romani · 2024-10-02T19:33:29Z

Google style use false, checkstyle style is true.
We need to improve logic for allowNewlineParagraph to do not violate our javadocs that are keep paragraph tag alone on line.

Zopsss · 2024-10-02T21:01:20Z

config/checkstyle-checks.xml

+  <!--
+    Until we fully support opening-closing paragraph tag
+    https://github.com/checkstyle/checkstyle/issues/15685
+   -->
+  <module name="SuppressionSingleFilter">
+    <property name="checks" value="JavadocParagraph"/>
+    <property name="message" value="tag should be preceded with an empty line."/>
+  </module>
+  <module name="SuppressionSingleFilter">
+    <property name="checks" value="JavadocParagraph"/>
+    <property name="message" value="Redundant .* tag."/>
+  </module>


suppression until we fix the implementation of this check. May change the referenced issue in future if we open new issue for it

We need to fix almost all violations from Paragraph Check.

checkstyle/src/main/java/com/puppycrawl/tools/checkstyle/JavaAstVisitor.java

Lines 84 to 86 in aeaf4b0

* </pre>

* <p>

* See <a href="https://github.com/checkstyle/checkstyle/pull/10434">#10434</a>

is good example. Please start a process of updating our sources, let's make PRs to be limited to ~20-30 files. All PRs will have "supplemental: " prefix.

If there is some strange cases, report to us and skip them for now, we will handle them at the end of as soon as you get confirmation on how to update javadoc.

we need fixes, please continue #15737 and all other fixes.
We eat food we produce before we release to it to users, we need to fix all of them.

Put this link in comment , you will update all in separate PR

Zopsss · 2024-10-03T10:32:58Z

A PR has been sent for failing CI: https://checkstyle.semaphoreci.com/jobs/46bc3abc-9190-43d9-bb99-3a25a47a386b

sevntu-checkstyle/sevntu.checkstyle#1037

Zopsss · 2024-10-03T11:24:20Z

fixed some bug related to <p> tag should be placed immediately before the first word in the latest push: https://github.com/checkstyle/checkstyle/compare/643b58d3d4f9225d764a9a064a5aa5e5eac69503..c80a41ccedf6312677b6578b19091f815bb3d1e1#diff-19c14e011c79f51ad2e551f4941a1def0d5ee70c40d93fd944e07da166bad4bb

romani · 2024-10-03T12:40:54Z

@Zopsss , please send PR to pgjdbc project to fix violations.

Zopsss · 2024-10-03T14:19:19Z

Build is flooded with errors again :(

https://app.circleci.com/pipelines/github/checkstyle/checkstyle/28127/workflows/60b27b2a-fb0d-4774-a64f-b37dea3364ca/jobs/698909

this time it is because of following type of violation:

<p> tag should not precede HTML block-tag '<ul>', <p> tag should be removed. [JavadocParagraph]

should I add suppression for this too?

Zopsss · 2024-10-03T15:07:02Z

GitHub, generate report for configs in PR description

romani · 2024-10-03T15:11:05Z

This PR is unblocked and will be final functional update for this Check.
There will be more attention to diff report.

github-actions · 2024-10-03T15:55:41Z

https://checkstyle-diff-reports.s3.us-east-2.amazonaws.com/88c23c4e17b959575de5fccbe349c1e7d06b312d_2024155512/reports/diff/index.html

Zopsss · 2024-10-04T06:44:57Z

Addressed #15732 in the latest push: https://github.com/checkstyle/checkstyle/compare/88c23c4e17b959575de5fccbe349c1e7d06b312d..1042402e60a306559e7a2f263389b8ec7d91ddaa

ignore failing build, reason explained at: #15686 (comment)

romani

items:

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

Zopsss · 2024-10-10T13:45:09Z

not sure why ci is failing

https://app.circleci.com/pipelines/github/checkstyle/checkstyle/28232/workflows/df53342b-c53d-431f-9c77-888fd26fff20/jobs/703338

local build was passing.

Zopsss · 2024-10-10T13:47:18Z

Github, generate site

romani · 2024-10-10T15:15:14Z

https://ci.appveyor.com/project/Checkstyle/checkstyle/builds/50771010/job/410wglmmfwmaa7nr#L254

[ERROR] Failed to execute goal org.apache.maven.plugins:maven-site-plugin:3.12.1:site (default-site) on project checkstyle: Error parsing 'C:\projects\checkstyle\src\xdocs\checks\javadoc\javadocparagraph.xml': Error validating the model: Error:
[ERROR]   Public ID: null
[ERROR]   System ID: null
[ERROR]   Line number: 34
[ERROR]   Column number: 15
[ERROR]   Message: cvc-complex-type.2.4.a: Invalid content was found starting with element '{"http://maven.apache.org/XDOC/2.0":ul}'. 
One of  ....
Is expected.

It prints allowed tags , see all in link

romani

Items

src/xdocs/checks/javadoc/javadocparagraph.xml.template

romani · 2024-10-10T16:52:48Z

Github, generate site

romani · 2024-10-10T16:55:33Z

Github, generate report for JavadocParagraph/Example2

Zopsss · 2024-10-10T16:55:41Z

GitHub, generate report for JavadocParagraph/all-examples-in-one

github-actions · 2024-10-10T17:50:43Z

https://checkstyle-diff-reports.s3.us-east-2.amazonaws.com/5a6b51e_2024174820/index.html

https://checkstyle-diff-reports.s3.us-east-2.amazonaws.com/5a6b51e_2024174820/checks/javadoc/javadocparagraph.html#Description

github-actions · 2024-10-10T18:32:20Z

Report for JavadocParagraph/all-examples-in-one:
https://checkstyle-diff-reports.s3.us-east-2.amazonaws.com/5a6b51e9b37f1087ae00db7ea83a18403b4e935b_2024183141/reports/diff/index.html

github-actions · 2024-10-10T18:33:47Z

Report for JavadocParagraph/Example2:
https://checkstyle-diff-reports.s3.us-east-2.amazonaws.com/5a6b51e9b37f1087ae00db7ea83a18403b4e935b_2024183253/reports/diff/index.html

romani

Ok to merge

rdiachenko

Minor note, ok to merge after it is fixed.

rdiachenko · 2024-10-11T15:50:19Z

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

+     * @return true, if the paragraph tag is nested.
+     */
+    private static boolean isNestedParagraph(DetailNode tag) {
+        boolean isNested = false;


Please rename to nested, it's a variable name, shouldn't be a verb.

…g when they have their corresponding closing tag

Zopsss · 2024-10-11T17:27:17Z

Check no closed issue reference failing CI is addressed at #15765

romani

I see fix for above item.

… for checkstyle#15686

Zopsss force-pushed the 15685-fixing-implementation branch from c88060a to c23ec48 Compare September 24, 2024 09:50

Zopsss force-pushed the 15685-fixing-implementation branch 2 times, most recently from eff31fd to fd5db92 Compare September 27, 2024 18:07

romani added the blocked label Sep 28, 2024

Zopsss force-pushed the 15685-fixing-implementation branch from fd5db92 to a358f05 Compare October 2, 2024 18:08

Zopsss force-pushed the 15685-fixing-implementation branch from a358f05 to 643b58d Compare October 2, 2024 21:00

Zopsss commented Oct 2, 2024

View reviewed changes

Zopsss mentioned this pull request Oct 3, 2024

minor: fixed P tag content's placement in CauseParameterInExceptionCheck.java sevntu-checkstyle/sevntu.checkstyle#1037

Merged

Zopsss force-pushed the 15685-fixing-implementation branch from 643b58d to c80a41c Compare October 3, 2024 11:23

Zopsss force-pushed the 15685-fixing-implementation branch from c80a41c to 88c23c4 Compare October 3, 2024 14:10

romani removed the blocked label Oct 3, 2024

Zopsss force-pushed the 15685-fixing-implementation branch from 88c23c4 to 1042402 Compare October 4, 2024 06:44

romani requested changes Oct 10, 2024

View reviewed changes

src/main/java/com/puppycrawl/tools/checkstyle/checks/javadoc/JavadocParagraphCheck.java Outdated Show resolved Hide resolved

src/main/java/com/puppycrawl/tools/checkstyle/checks/javadoc/JavadocParagraphCheck.java Outdated Show resolved Hide resolved

Zopsss force-pushed the 15685-fixing-implementation branch from c0d510e to cb7cac3 Compare October 10, 2024 13:41

romani requested changes Oct 10, 2024

View reviewed changes

src/xdocs/checks/javadoc/javadocparagraph.xml.template Outdated Show resolved Hide resolved

Zopsss force-pushed the 15685-fixing-implementation branch from cb7cac3 to dedae08 Compare October 10, 2024 16:11

Zopsss mentioned this pull request Oct 10, 2024

Clarification: no violation for preceding whitespaces for paragraph tag content #15762

Closed

Zopsss force-pushed the 15685-fixing-implementation branch from dedae08 to 5a6b51e Compare October 10, 2024 17:12

Zopsss force-pushed the 15685-fixing-implementation branch 2 times, most recently from 116c234 to bc5dfcf Compare October 10, 2024 21:55

romani approved these changes Oct 10, 2024

View reviewed changes

romani requested a review from rdiachenko October 10, 2024 21:57

romani assigned rdiachenko and unassigned romani Oct 10, 2024

rdiachenko approved these changes Oct 11, 2024

View reviewed changes

Issue checkstyle#15685: fixed JavadocParagraph to detect paragraph ta…

ca20d44

…g when they have their corresponding closing tag

Zopsss force-pushed the 15685-fixing-implementation branch from bc5dfcf to ca20d44 Compare October 11, 2024 17:22

romani approved these changes Oct 11, 2024

View reviewed changes

romani merged commit 5b21ee5 into checkstyle:master Oct 11, 2024
111 of 112 checks passed

Zopsss deleted the 15685-fixing-implementation branch October 11, 2024 20:16

MohanadKh03 pushed a commit to MohanadKh03/checkstyle that referenced this pull request Oct 18, 2024

infra: move pgjdbc to jdk17 to allow usage of latest commit, required…

48235d2

… for checkstyle#15686

MohanadKh03 pushed a commit to MohanadKh03/checkstyle that referenced this pull request Oct 18, 2024

infra: move pgjdbc to jdk17 to allow usage of latest commit, required…

ba28df9

… for checkstyle#15686

	* </pre>
	* <p>
	* See <a href="https://github.com/checkstyle/checkstyle/pull/10434">#10434</a>

Issue #15685: fixed JavadocParagraph to detect paragraph tag when they have their corresponding closing tag #15686

Issue #15685: fixed JavadocParagraph to detect paragraph tag when they have their corresponding closing tag #15686

Conversation

Zopsss commented Sep 21, 2024 • edited Loading

Zopsss commented Sep 24, 2024

romani commented Sep 26, 2024

Zopsss commented Sep 27, 2024

romani commented Sep 28, 2024

romani commented Oct 2, 2024

Zopsss commented Oct 2, 2024

Zopsss commented Oct 2, 2024

romani commented Oct 2, 2024

Zopsss commented Oct 2, 2024

romani commented Oct 2, 2024

Zopsss Oct 2, 2024

Choose a reason for hiding this comment

romani Oct 4, 2024 • edited Loading

Choose a reason for hiding this comment

romani Oct 4, 2024

Choose a reason for hiding this comment

romani Oct 10, 2024

Choose a reason for hiding this comment

romani Oct 10, 2024

Choose a reason for hiding this comment

Zopsss commented Oct 3, 2024

Zopsss commented Oct 3, 2024 • edited Loading

romani commented Oct 3, 2024

Zopsss commented Oct 3, 2024

Zopsss commented Oct 3, 2024

romani commented Oct 3, 2024

github-actions bot commented Oct 3, 2024

Zopsss commented Oct 4, 2024 • edited Loading

romani left a comment

Choose a reason for hiding this comment

Zopsss commented Oct 10, 2024

Zopsss commented Oct 10, 2024

romani commented Oct 10, 2024 • edited Loading

romani left a comment

Choose a reason for hiding this comment

romani commented Oct 10, 2024

romani commented Oct 10, 2024

Zopsss commented Oct 10, 2024

github-actions bot commented Oct 10, 2024

github-actions bot commented Oct 10, 2024

github-actions bot commented Oct 10, 2024

romani left a comment

Choose a reason for hiding this comment

rdiachenko left a comment

Choose a reason for hiding this comment

rdiachenko Oct 11, 2024

Choose a reason for hiding this comment

Zopsss Oct 11, 2024

Choose a reason for hiding this comment

Zopsss commented Oct 11, 2024

romani left a comment

Choose a reason for hiding this comment

Zopsss commented Sep 21, 2024 •

edited

Loading

romani Oct 4, 2024 •

edited

Loading

Zopsss commented Oct 3, 2024 •

edited

Loading

Zopsss commented Oct 4, 2024 •

edited

Loading

romani commented Oct 10, 2024 •

edited

Loading