Pavel,
Thanks for the detailed comments. I'll go through the list and respond
in detail when I have the code and review in front of me.
I also have in mind some more detailed class-level doc comments for
BaseOptions and HtmlOptions classes that I will put in the next round.
-- Jon
On 1/21/20 4:24 AM, Pavel Rappo wrote:
Jon,
I don't have a strong opinion on this refactoring. I will rely on your judgment
and the fact that you know that code well.
My initial reaction was "Isn't it violating the Law of Demeter or something?"
But on a closer look, I understood that it was merely a restructuring action.
What you seem to be doing is trying to compartmentalize that code better.
This refactoring preserves the collocation that exists between the code that
parses the properties and the code that provides these properties for internal
consumption.
1. You have reintroduced a forgotten bounded wildcard to
public Set<? extends Doclet.Option> getSupportedOptions()
Good. Compatibility-wise this should be benign. Hopefully, no one tries to put
anything into that set, which should be assumed unmodifiable anyway.
2. You consistently used camelCase naming for fields that represent options.
This effectively "unlinks" them from their command-line names, which is not bad.
Fewer possibilities to mess this during (automated) future refactorings if you
ask me.
3. I'm not sure what can be done about that TreeSet on HtmlOptions.java:458.
Statically it operates on a non-comparable type, Doclet.Option.
4. What is the purpose of the BaseOptions.getOptions() method?
5. HtmlConfiguration:170 feels like a debugging leftover. I'm not sure that
this condition is even possible. Could it be an `assert` instead?
6. AbstractMemberWriter's fields `printedSummaryHeader` and `nodepr` seem not to
be used. Can those be deleted?
7. AbstractMemberWriter.java: 385, 603. Those could use the internal (alias)
field `options`.
8. I noticed that BaseConfiguration have many blank lines. Was it done on
purpose?
9. Should BaseOptions use 2020 as the second copyright year?
While we are in this area, consider hyphenating "command line" where it is a
compound adjective rather than a noun (possibly, not an exhaustive list):
* HtmlConfiguration: 54, 56
* HtmlOptions: 68, 74, 87, 125, 132, 138, 144, 162
* BaseConfiguration: 396, 693
* BaseOptions: 178
* IllegalOptionValue: 32
* Main: 49, 58, 70, 83
* javadoc.properties: 94
-Pavel
P.S. Thanks for being super careful and not only updating the javadoc comments
but also the commented out code in SourceToHTMLConverter!
On 18 Jan 2020, at 01:51, Jonathan Gibbons <[email protected]> wrote:
Please review a code-cleanup to reorganize the code to handle doclet options.
Fundamentally, the change is to move the methods and fields to handle option
processing from BaseConfiguration and HtmlConfiguration out into new
abstractions BaseOptions and HtmlOptions. As such, the dominant changes are to
these 4 files.
The impact on the rest of the doclet code is just to change where to access the
values for options: instead of accessing the values directly from the
*Configuration classes, the values are now obtained from the corresponding
*Option classes, which are in turn accessed from the *Configuration classes.
The reference to the Options objects are typically cached when there are a
large number of usages in the code. In a number of cases, the cache is in a
supertype, which reduces the visible churn.
I've taken this opportunity to rename the fields for the values of options into
the standard camelCase convention. And, I've done some basic work to clean up
comments, although more could be done (later).
Fixing a bunch of spurious warnings uncoverable a real warning, that the code
was creating a sorted set of incomparable Option objects. This changeset also
fixes that issue, which mostly just shows up in the signatures for internal
collections of option objects.
There is no change in functionality here. All the tests pass without change,
except for one that was tunneling into an impl class, and which needed to be
updated.
There's probably a similar cleanup coming to the code to handle tool options.
-- Jon
JBS: https://bugs.openjdk.java.net/browse/JDK-8237492
Webrev: http://cr.openjdk.java.net/~jjg/8237492/webrev/
