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.

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

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

Conversation

patchwork01
Copy link
Contributor

@patchwork01 patchwork01 commented Mar 19, 2024

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.

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

jp-period-Diff Regression config: https://gist.githubusercontent.com/romani/3a9b1b47b71ad0a9f5583a738ff4710c/raw/862494acad8497fb3129d7893dd1438ebf48c7b8/my_check-pr-14690.xml
jp-period-Diff Regression projects: https://gist.githubusercontent.com/romani/edc0f152b7dd10118e6bc395bb1e62af/raw/c8cc30b749d4c27b94fa219b701120f1bca4a435/pull-14690-projects.properties

Prev-Diff Regression config: https://gist.githubusercontent.com/patchwork01/4bcb6b58b28420050538692ddedb0938/raw/4a75a7edd7e01edd0fda401cb9496f5e4b8b51b7/my_check.xml

jp-period-Diff Regression config: https://gist.githubusercontent.com/romani/3a9b1b47b71ad0a9f5583a738ff4710c/raw/862494acad8497fb3129d7893dd1438ebf48c7b8/my_check-pr-14690.xml

Diff Regression config: https://raw.githubusercontent.com/checkstyle/test-configs/main/SummaryJavadoc/japan-period/config.xml
Diff Regression projects: https://raw.githubusercontent.com/checkstyle/test-configs/main/SummaryJavadoc/japan-period/list-of-projects.properties

@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
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:

@romani
Copy link
Member

romani commented Mar 25, 2024

@patchwork01, please resolve Checker and Pitest failures.

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

@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

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

GitHub, generate report

Copy link
Contributor

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

@rnveach
Copy link
Member

rnveach commented May 18, 2024

Regression only ran against default SummaryJavadoc. We need run with change in property.

@romani
Copy link
Member

romani commented May 18, 2024

The only meaningful config we can run on is https://github.com/checkstyle/checkstyle/pull/14690/files#diff-86ad081949df60d8dd42db875094b5df6d70aed3f23759b7520605bfc23f86beR5
But we do not have Japanese open source projects in list, so no changes will be.
We can run, will it be enough?

If not, please share what configs you think we need to run on

by means of https://github.com/search?q=+language%3AJava++%E3%80%82&type=code
I found: https://github.com/Wechat-Group/WxJava

@romani
Copy link
Member

romani commented May 18, 2024

Github, generate report

@rnveach
Copy link
Member

rnveach commented May 20, 2024

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

Report should have all projects.

please share what configs you think we need to run on

Use the same config, but also include the US period. Based on the logic presented here, it will assume it has a space after it so I should differences between this and the other config with the default.

@patchwork01
Copy link
Contributor Author

Use the same config, but also include the US period. Based on the logic presented here, it will assume it has a space after it so I should differences between this and the other config with the default.

The US period is the default. That's the configuration that was used in this run 5 days ago. We can set the period character explicitly, but it won't be any different.

@patchwork01
Copy link
Contributor Author

Github, generate report

@patchwork01
Copy link
Contributor Author

GitHub, generate report

Copy link
Contributor

Report generation failed on phase "make_report",
step "Generate report".
Link: https://github.com/checkstyle/checkstyle/actions/runs/9171202334

Copy link
Contributor

Report generation failed on phase "make_report",
step "Generate report".
Link: https://github.com/checkstyle/checkstyle/actions/runs/9171243385

@patchwork01 patchwork01 force-pushed the javadoc-summary-should-only-be-first-sentence branch from 3043b4f to 84bd50f Compare May 21, 2024 09:12
@patchwork01
Copy link
Contributor Author

GitHub, generate report

@rnveach
Copy link
Member

rnveach commented May 21, 2024

We can set the period character explicitly, but it won't be any different.

My fault, I missed this in the documentation.

@rnveach
Copy link
Member

rnveach commented May 21, 2024

Please provide the regression of the japanese period on all projects.

@romani
Copy link
Member

romani commented May 21, 2024

Semaphore is restarted, it was real problem after merge of unrelated PR. Should be green after restart

@romani
Copy link
Member

romani commented May 21, 2024

GitHub, generate report

Copy link
Contributor

Report generation failed on phase ,
step .
Link: https://github.com/checkstyle/checkstyle/actions/runs/9177354955

@romani
Copy link
Member

romani commented May 21, 2024

GitHub, generate report

Copy link
Contributor

Report generation failed on phase ,
step .
Link: https://github.com/checkstyle/checkstyle/actions/runs/9177422046

@romani
Copy link
Member

romani commented May 21, 2024

GitHub, generate report

@romani
Copy link
Member

romani commented May 21, 2024

@rnveach , done, no changes.

@romani
Copy link
Member

romani commented May 31, 2024

@rnveach , please finish review

@romani
Copy link
Member

romani commented Jun 1, 2024

GitHub, generate report

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants