-
Notifications
You must be signed in to change notification settings - Fork 488
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
8250590: Classes and methods in the javafx.css package are missing documentation #589
Conversation
👋 Welcome back aghaisas! A progress list of the required criteria for merging this PR into |
Webrevs
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I applied the patch and confirm that there are no more "missing comments" warnings for the javafx.css
package.
The docs look reasonable in general. I did note a few examples of usage patterns that should be fixed globally. Most of these fell into the following general categories.
- The body of the javadoc comments for a class, method, etc., should be one or more complete sentences starting with a capital letter and ending with a period.
- The text following
@return
and@param
typically starts with a lower-case letter and does not end with a period. - When referring to a class name, method name, or formal parameter name, we usually use
{@code ... }
style or else separate words that are all lower-case. For example, either{@code ParsedValue}
orparsed value
is fine (but notParsedValue
).
In addition to noting one or two examples of each of the above, I left a few random inline comments.
modules/javafx.graphics/src/main/java/javafx/css/CssParser.java
Outdated
Show resolved
Hide resolved
modules/javafx.graphics/src/main/java/javafx/css/CssParser.java
Outdated
Show resolved
Hide resolved
modules/javafx.graphics/src/main/java/javafx/css/CssParser.java
Outdated
Show resolved
Hide resolved
modules/javafx.graphics/src/main/java/javafx/css/CssParser.java
Outdated
Show resolved
Hide resolved
modules/javafx.graphics/src/main/java/javafx/css/SizeUnits.java
Outdated
Show resolved
Hide resolved
modules/javafx.graphics/src/main/java/javafx/css/StyleClass.java
Outdated
Show resolved
Hide resolved
modules/javafx.graphics/src/main/java/javafx/css/StyleClass.java
Outdated
Show resolved
Hide resolved
modules/javafx.graphics/src/main/java/javafx/css/StyleClass.java
Outdated
Show resolved
Hide resolved
modules/javafx.graphics/src/main/java/javafx/css/converter/EffectConverter.java
Show resolved
Hide resolved
A second pair of eyes would be good for these changes. /reviewers 2 |
@kevinrushforth |
I have spotted some missing docs:
I stopped checking at this point. It seems that the the script warning for missing docs isn't looking at overriden methods. |
I understand we discussed this in the meeting, but I am surprised that this does not need a CSR. It looks like too much bigger documentation change for not needing a CSR. Besides this, I have given some minor comments, otherwise it looks fine to me. |
JavaFX doesn't impose the same requirement of needing a CSR for everything that touches API documentation as does the JDK. In this case, it is just adding obvious and missing API docs without any changes in documented behavior or any additional conditions in the specification. While it wouldn't hurt to have a CSR, I'm not sure I see the need. Btw, the number of lines of documentation added isn't really the question. A one-line change can end up needing a CSR if there are changes to the specified semantics. |
I'm not too worried about the missing docs for overridden methods. Unless we are overriding it in a way that needs to be documented, we don't want or need to add any explicit documentation. We could add
I think |
There could be more methods like that. Maybe the script should check specifically for a missing summary? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did a pretty complete review of the docs, and left what are mostly minor (and in some cases picky) comments.
modules/javafx.graphics/src/main/java/javafx/css/CssParser.java
Outdated
Show resolved
Hide resolved
modules/javafx.graphics/src/main/java/javafx/css/Declaration.java
Outdated
Show resolved
Hide resolved
modules/javafx.graphics/src/main/java/javafx/css/Declaration.java
Outdated
Show resolved
Hide resolved
modules/javafx.graphics/src/main/java/javafx/css/Declaration.java
Outdated
Show resolved
Hide resolved
modules/javafx.graphics/src/main/java/javafx/css/Declaration.java
Outdated
Show resolved
Hide resolved
modules/javafx.graphics/src/main/java/javafx/css/converter/EnumConverter.java
Outdated
Show resolved
Hide resolved
modules/javafx.graphics/src/main/java/javafx/css/converter/StringConverter.java
Outdated
Show resolved
Hide resolved
modules/javafx.graphics/src/main/java/javafx/css/converter/StringConverter.java
Outdated
Show resolved
Hide resolved
modules/javafx.graphics/src/main/java/javafx/css/converter/StringConverter.java
Outdated
Show resolved
Hide resolved
modules/javafx.graphics/src/main/java/javafx/css/converter/URLConverter.java
Outdated
Show resolved
Hide resolved
We just used the |
I found few more such methods in javafx.css package using "find in files" in my editor and fixed them.
Interestingly, this is not only limited to javafx.css package, but spread across all JavaFX modules. |
Thanks for a detailed review of the changes. |
I see that this is a much wider problem with openjfx17 documentation. Need to find root cause and fix this. |
I saw this problem with |
...
These three are not part of the public API. |
I can confirm that this is a |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I left just a few more (mostly minor) comments.
modules/javafx.graphics/src/main/java/javafx/css/Declaration.java
Outdated
Show resolved
Hide resolved
modules/javafx.graphics/src/main/java/javafx/css/converter/EffectConverter.java
Show resolved
Hide resolved
modules/javafx.graphics/src/main/java/javafx/css/Declaration.java
Outdated
Show resolved
Hide resolved
modules/javafx.graphics/src/main/java/javafx/css/Declaration.java
Outdated
Show resolved
Hide resolved
I will add the documentation for this method. |
I have filed JDK-8271485 to address this. |
I just submitted PR #593 with a fix. It will also address the problem noted above of overridden methods without additional documentation showing up with no description, since such methods will no longer show up in the first place. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Two remaining comments. I looked through all the generated docs and everything else looks good.
modules/javafx.graphics/src/main/java/javafx/css/PseudoClass.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me as well
@aghaisas This change now passes all automated pre-integration checks. ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details. After integration, the commit message for the final commit will be:
You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed. At the time when this comment was updated there had been 5 new commits pushed to the
Please see this link for an up-to-date comparison between the source branch of this pull request and the ➡️ To integrate this PR with the above commit message to the |
/integrate |
Going to push as commit 7575473.
Your commit was automatically rebased without conflicts. |
This PR corrects/adds missing documentation for classes in javafx.css package.
Progress
Issue
Reviewers
Reviewing
Using
git
Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jfx pull/589/head:pull/589
$ git checkout pull/589
Update a local copy of the PR:
$ git checkout pull/589
$ git pull https://git.openjdk.java.net/jfx pull/589/head
Using Skara CLI tools
Checkout this PR locally:
$ git pr checkout 589
View PR using the GUI difftool:
$ git pr show -t 589
Using diff file
Download this PR as a diff file:
https://git.openjdk.java.net/jfx/pull/589.diff