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

Issue #14689: Prevent false positives when first sentence of Javadoc is on its own line #14690

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Issue #14689: Prevent false positives when first sentence of Javadoc is on its own line #14690

wants to merge 1 commit into from
+210 −59

Conversation

patchwork01
Copy link
Contributor

@patchwork01 patchwork01 commented Mar 19, 2024 •
edited

Resolves: #14689
Resolves: #14750
Resolves: #14751

Handle cases where first sentence of Javadoc is on its own line.

Handle cases where first sentence of Javadoc includes a period character without whitespace after it.

Diff Regression config: https://gist.githubusercontent.com/patchwork01/d62651d8467212daf75aff8100e813c1/raw/ac03b5b7fbcbee95f7508e40462ce97d72dfc550/my_check.xml

@patchwork01 patchwork01 force-pushed the javadoc-summary-should-only-be-first-sentence branch 4 times, most recently from 69c7787 to a262f21 Compare March 20, 2024 09:43
@romani
Copy link
Member

romani commented Mar 22, 2024

please squash all in single commit

@patchwork01 patchwork01 force-pushed the javadoc-summary-should-only-be-first-sentence branch 2 times, most recently from 4310b16 to c311d18 Compare March 22, 2024 10:19
@patchwork01
Copy link
Contributor Author

please squash all in single commit

Done

@romani romani force-pushed the javadoc-summary-should-only-be-first-sentence branch from c311d18 to 02f844a Compare March 22, 2024 13:18
romani
romani requested changes Mar 22, 2024
View reviewed changes
Copy link
Member

@romani romani left a comment

Choose a reason for hiding this comment

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

thanks a lot for fix.

first shallow dive in PR:

...com/puppycrawl/tools/checkstyle/checks/javadoc/summaryjavadoc/InputSummaryJavadocPeriod.java Outdated Show resolved Hide resolved
...ckstyle/test/chapter7javadoc/rule72thesummaryfragment/InputIncorrectSummaryJavaDocCheck.java Outdated Show resolved Hide resolved
...om/puppycrawl/tools/checkstyle/checks/javadoc/summaryjavadoc/InputSummaryJavadocCorrect.java Outdated Show resolved Hide resolved
@romani
Copy link
Member

romani commented Mar 25, 2024

@patchwork01, please resolve Checker and Pitest failures.

romani
romani requested changes Mar 25, 2024
View reviewed changes
Copy link
Member

@romani romani left a comment

Choose a reason for hiding this comment

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

Items

src/main/java/com/puppycrawl/tools/checkstyle/checks/javadoc/SummaryJavadocCheck.java Outdated Show resolved Hide resolved
@patchwork01 patchwork01 force-pushed the javadoc-summary-should-only-be-first-sentence branch 8 times, most recently from 741f2e9 to 6cfadae Compare March 28, 2024 11:40
@romani
Copy link
Member

romani commented Mar 28, 2024 •
edited

please review if we can avoid this: https://github.com/checkstyle/checkstyle/actions/runs/8466706931/job/23196469563?pr=14690#step:6:933

New surviving error(s) found:

  <checkerFrameworkError unstable="false">
    <fileName>src/main/java/com/puppycrawl/tools/checkstyle/checks/javadoc/SummaryJavadocCheck.java</fileName>
    <specifier>argument</specifier>
    <message>incompatible argument for parameter end of StringBuilder.append.</message>
    <lineContent>result.append(text, 0, periodIndex);</lineContent>
    <details>
      found   : int
      required: @LTEqLengthOf(&quot;text&quot;) int
    </details>
  </checkerFrameworkError>

if not, just run groovy ./.ci/checker-framework.groovy checker-index on your local and add generated update/change to suppression file to your PR. It will be red flag for each reviewer, so better to share reasons why we cannot fix it, or it is not reasonable, or ... .

@romani
Copy link
Member

romani commented Mar 28, 2024

last testing request, please read https://github.com/checkstyle/contribution/tree/master/checkstyle-tester#basic-difference-report-with-custom-projects-list and provide such two configs and lets test on bunch of real code. Result will be diff report, we need o make sure there is not unexpected regressions.

@patchwork01 patchwork01 force-pushed the javadoc-summary-should-only-be-first-sentence branch 3 times, most recently from 2afe320 to 0563051 Compare March 30, 2024 10:16
@patchwork01
Copy link
Contributor Author

please review if we can avoid this: https://github.com/checkstyle/checkstyle/actions/runs/8466706931/job/23196469563?pr=14690#step:6:933

That's fixed now.

last testing request, please read https://github.com/checkstyle/contribution/tree/master/checkstyle-tester#basic-difference-report-with-custom-projects-list and provide such two configs and lets test on bunch of real code. Result will be diff report, we need o make sure there is not unexpected regressions.

I'm not sure what you mean. What do you want me to do?

@romani
Copy link
Member

romani commented Apr 1, 2024

please read https://github.com/checkstyle/contribution/tree/master/checkstyle-tester#executing-generation

Example configuration in PR description and triggering for different Check - #14743

@patchwork01
Copy link
Contributor Author

patchwork01 commented Apr 2, 2024 •
edited

GitHub, generate report

@romani
Copy link
Member

romani commented Apr 21, 2024

GitHub, generate report

@github-actions GitHub Actions
Copy link
Contributor

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

@patchwork01 patchwork01 force-pushed the javadoc-summary-should-only-be-first-sentence branch 2 times, most recently from e340daa to cc00495 Compare April 25, 2024 15:23
@romani
Copy link
Member

romani commented Apr 25, 2024

CI is red

@patchwork01 patchwork01 force-pushed the javadoc-summary-should-only-be-first-sentence branch 3 times, most recently from ab1773f to 9a336b9 Compare April 26, 2024 09:25
romani
romani approved these changes Apr 28, 2024
View reviewed changes
Copy link
Member

@romani romani left a comment

Choose a reason for hiding this comment

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

ok to merge.
Thanks a lot for your contribution

@romani romani assigned nrmancuso and unassigned romani Apr 28, 2024
@romani romani requested a review from nrmancuso April 28, 2024 11:31
@romani
Copy link
Member

romani commented Apr 28, 2024

Github, generate website

@github-actions GitHub Actions
Copy link
Contributor

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

https://checkstyle-diff-reports.s3.us-east-2.amazonaws.com/9a336b9_2024113540/checks/javadoc/summaryjavadoc.html#Properties

nrmancuso
nrmancuso requested changes May 1, 2024
View reviewed changes
Copy link
Member

@nrmancuso nrmancuso left a comment

Choose a reason for hiding this comment

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

First pass:

src/main/java/com/puppycrawl/tools/checkstyle/checks/javadoc/SummaryJavadocCheck.java Outdated Show resolved Hide resolved
src/main/java/com/puppycrawl/tools/checkstyle/checks/javadoc/SummaryJavadocCheck.java Outdated Show resolved Hide resolved
src/main/java/com/puppycrawl/tools/checkstyle/checks/javadoc/SummaryJavadocCheck.java Outdated Show resolved Hide resolved
src/main/java/com/puppycrawl/tools/checkstyle/checks/javadoc/SummaryJavadocCheck.java Outdated Show resolved Hide resolved
src/main/java/com/puppycrawl/tools/checkstyle/checks/javadoc/SummaryJavadocCheck.java Outdated Show resolved Hide resolved
@patchwork01 patchwork01 force-pushed the javadoc-summary-should-only-be-first-sentence branch 2 times, most recently from d3a2680 to fc3e4f6 Compare May 3, 2024 15:46
@romani
Copy link
Member

romani commented May 3, 2024

To fix CI please rebase on latest

@patchwork01 patchwork01 force-pushed the javadoc-summary-should-only-be-first-sentence branch 9 times, most recently from 71ec4e7 to 2706e2c Compare May 3, 2024 19:19
@patchwork01 patchwork01 force-pushed the javadoc-summary-should-only-be-first-sentence branch from 2706e2c to b48508b Compare May 3, 2024 19:25
nrmancuso
nrmancuso requested changes May 7, 2024
View reviewed changes
Copy link
Member

@nrmancuso nrmancuso left a comment

Choose a reason for hiding this comment

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

Last from me:

src/main/java/com/puppycrawl/tools/checkstyle/checks/javadoc/SummaryJavadocCheck.java
* @param period The period character to find.
* @return The string up to and excluding the period, if one was found.
*/
private static Optional<String> findSentenceEnding(String text, String period) {
Copy link
Member

Choose a reason for hiding this comment

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

Why return an optional from here if the only usage we use .orElse(null)?

src/main/java/com/puppycrawl/tools/checkstyle/checks/javadoc/SummaryJavadocCheck.java
final String text;
if (child.getChildren().length == 0) {
text = child.getText();
private static Optional<String> getFirstSentence(DetailNode ast, String period) {
Copy link
Member

Choose a reason for hiding this comment

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

Why return an optional from here if the only usage we use .orElse(null)?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nrmancuso nrmancuso nrmancuso requested changes

@romani romani romani approved these changes

Assignees

@nrmancuso nrmancuso

Labels
None yet
Projects
None yet
Milestone
No milestone
3 participants
@patchwork01 @romani @nrmancuso