Issue #19800: Add Doc Comments Style Guide coverage infrastructure#19801
Conversation
|
GitHub, generate website |
cee7225 to
eaf23e6
Compare
|
GitHub, generate website |
There was a problem hiding this comment.
The doc-comments-style folder name should include either it's version or latest release date like google & openjdk style guide has.
There was a problem hiding this comment.
I was not able to find any reliable information about the latest version number or release date for this style guide. So for now, let's keep the doc-comments-style folder name as it is. In the future, if we find proper versioning or release information and need to support different versions, then we should update the naming accordingly.
There was a problem hiding this comment.
Yes exactly, I was looking for some info but didn't find any. I think this is from Sun from 2000, but I don't know if Oracle has modified it since then
There was a problem hiding this comment.
Please remove this file. All style guides have their own .js & .css files and they are copied from their official GitHub repo, these are used so we can re-create the cache page with exact UI of the original documentation.
I think you have copied this .js file from other style guides, am I right? Please correct me if I'm wrong.
If you have copied it from other style guides then we should remove it, and only keep the .html file.
There was a problem hiding this comment.
they are copied from their official GitHub repo
I think that openjdk's JS is copied from the google's one, the original openjdk web page doesn't have the link image we use. Also the two JS files look practically the same.
think you have copied this
.jsfile from other style guides, am I right?
Yes exactly, I thought we did the same for openjdk.
If you have copied it from other style guides then we should remove it, and only keep the
.htmlfile.
Ok, I'll keep only the html file. The JS was only adding the link image in practice, not so useful
There was a problem hiding this comment.
Yes exactly, I thought we did the same for openjdk.
I didn't know about openjdk's files. Thanks for pointing this, I'll check that out too.
Ok, I'll keep only the html file. The JS was only adding the link image in practice, not so useful
Please remove that link image as well. For now, let's only keep the .html file.
There was a problem hiding this comment.
from where did you find this html file?
There was a problem hiding this comment.
It is the html of the oracle web page. I adjusted it because it had to be compatible with toc creation and it also had lot of js
There was a problem hiding this comment.
I meant to ask that did you find it from a github repo or something?
There was a problem hiding this comment.
Nope, I saved the web page of oracle and then modified it with some LLM's help, it took a while. Content is the same though. Does this have copyright issues?
There was a problem hiding this comment.
Not sure about copyright issues but if you would have copied it from a Github repo then we would be able to keep track of its release date/version number.
There was a problem hiding this comment.
Ok, no it's not from a github repo
| final Node subSection = subSections.item(position); | ||
| final Node name = subSection.getAttributes().getNamedItem("name"); | ||
|
|
||
| assertWithMessage("All sub-sections in '%s' must have a name", fileName) |
There was a problem hiding this comment.
43 lines is the maximum allowed for the block, that's why I did that
| hasDocComments = true; | ||
| expectedUrl = "https://github.com/search?q=" | ||
| + "path%3Asrc%2Fmain%2Fresources%20path%3A**%2Fdoc_comments_checks.xml+" | ||
| + "repo%3Acheckstyle%2Fcheckstyle+" |
There was a problem hiding this comment.
The https://github.com/search?q=path%3Asrc%2Fmain%2Fresources%20path%3A**%2F${style guide name}repo%3Acheckstyle%2Fcheckstyle+ is common for all style guides, only the style guide name changes.
Please create a new method which accepts the style guide name and returns us the expectedUrl.
Example:
private static String getExpectedStyleGuideUrl(String styleGuideName) {
return "https://github.com/search?q=path%3Asrc%2Fmain%2Fresources%20path%3A**%2F"
+ styleGuideName
+ "repo%3Acheckstyle%2Fcheckstyle+";
}There was a problem hiding this comment.
There was a problem hiding this comment.
| .that(inputFolderUrl) | ||
| .startsWith("https://github.com/checkstyle/checkstyle/" | ||
| + "tree/master/src/it/resources/com/" + styleName | ||
| + "/checkstyle/test/"); | ||
| .that(inputFolderUrl) | ||
| .startsWith("https://github.com/checkstyle/checkstyle/" | ||
| + "tree/master/src/it/resources/com/" + stylePackageName | ||
| + "/checkstyle/test/"); |
There was a problem hiding this comment.
I did that because that test is trying to get the package name from the style name (doc_comments_style -> doc_comments), but actually I named the package as doccomments without underscore, so they won't match.
I didn't see underscores in our package names so I thought it is not desirable to have them. Let me know what you think
https://github.com/checkstyle/checkstyle/pull/19801/changes/BASE..c8f2da4bc13aa293727dbdcdb710782dcd092520#diff-b63e6b6ebb32f3256c7aba537acd42c86cc467b7ef91aec2eda8be13759f21c4R20
There was a problem hiding this comment.
I was pointing to the tab spaces you removed.
There was a problem hiding this comment.
@gianmarcoschifone , Please keep indentation as it was
|
GitHub, generate website |
|
Please add links to sections like I think almost all bold sentences should have anchors, to let us reference them more precisely. Each paragraph like this will be sliced and diced for requirements, so we need better granularity in reference. Highly likely each bold paragraph with be separate line in coverage table. |
Yes I agree. Issue: #19808 |
e464ac8 to
b884677
Compare
|
GitHub, generate website |
|
GitHub, generate website |
|
@gianmarcoschifone , make "Notes" with anchor. |
Ok no problem. My concern is that there could be lot of |
|
GitHub, generate website |
|
Github, validate links |
romani
left a comment
There was a problem hiding this comment.
Good base version.
We will improve a lot later on.
|
Linkcheck PASSED Run: https://github.com/checkstyle/checkstyle/actions/runs/25917802794 |
|
GitHub, generate website |
|
Last #19801 (comment) |
|
we lost all anchors in cached html. @Zopsss , ^^^ |
I'm sorry, what do you mean with anchors? Should I put the JS file back? |
Yes, we lost it after removing .js file. @gianmarcoschifone please add it back. |
These are the anchor links
Yes. |
|
CI failure is merged, it will be full green on rebase, lets ignore such failure for now and let @Zopsss finalize review |
|
GitHub, generate website |
|
@gianmarcoschifone , in next PR please remove bigital numbers from chapter of Doc style table, there no number, in original doc and we should not use them, we just use wording of bold text as visual anchor for humans. that user can copy and search inpage to locate it, if they do not trust our link.
can be shorted to |
|
Links works. |
Issue #19800
This PR adds the infrastructure for tracking Checkstyle coverage of Documentation Comments Java Style Guidelines.
The checks (existing and new) will be added in follow-up PRs as rules are mapped.