RFR: JDK-8296661 : Typo Found In CSSParser.java [v5]

Alexey Ivanov aivanov at openjdk.org
Wed Jan 25 19:32:52 UTC 2023


On Mon, 12 Dec 2022 13:06:09 GMT, Alexey Ivanov <aivanov at openjdk.org> wrote:

>> Looks good to me.
>> 
>> In the description above: “…invoked one per property…”, it shoud be “once”.
>> 
>> I think there's room for improvement. However, this is an internal-only class.
>
>> I think there's room for improvement. However, this is an internal-only class.
> 
> This is what I have in mind. The list of callbacks is inconsistent in how the method and conditions when it's called are listed. One of the list items has no ending punctuation. Some portions of text should use `<code>` or `{@code}`, or even [`<samp>`](https://developer.mozilla.org/en-US/docs/Web/HTML/Element/samp).
> 
> I think `<p>` should be added before this sentence, _“If an error results in parsing, a RuntimeException will be thrown.”_ The sentence itself requires re-writing: “If parsing results in an error…”
> 
> Both `RuntimeException` and `toLowerCase` below should be in `{@code}`.
> 
> The usage of `<code>` could be updated to `{@code}` which is the recommended way.
> 
> What do you think?

> @aivanov-jdk Done.

Sorry, @scientificware, I missed the update.

After making changes, you have to issue `integrate` command again.

In fact, I didn't mean to include [the changes I suggested](https://github.com/openjdk/jdk/pull/10975#issuecomment-1346460131) in this PR.

I still think there's room for improvement, however, it's not visible publicly. The list is inconsistent:

_“The delegate is notified of the following events:”_ — one would expect a list of events. Instead, each list item starts differently. If I may suggest, starting each item with a callback method followed by the condition when it's called and additional details will present a more consistent documentation. The introduction sentence before the list may also be updated.

-------------

PR: https://git.openjdk.org/jdk/pull/10975



More information about the client-libs-dev mailing list