> On 21 Jan 2020, at 18:55, Jonathan Gibbons <[email protected]>
> wrote:
>
> Pavel,
>
> All great feedback, and all points addressed, as described in the details
> inline below.
>
> New webrev, addresses all your comments, adds a couple of class-level doc
> comments
> to the two new classes, and fixes a couple of inconsequential spelling
> errors. Otherwise,
> no changes in all the other affected files.
>
> http://cr.openjdk.java.net/~jjg/8237492/webrev.01/
Thanks for patiently addressing my comments. I see this code review as an
opportunity to get familiar with the javadoc code base.
> <snip>
>
> The ongoing task is to draw lines around parts of the hodge-podge that is
> {Base,Html}Configuration, and to pull out those parts into separate, better
> defined abstractions.
A noble intent.
> <snip>
>
>> 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.
>
> It's only an internal API, and we control the implementations. As they say,
> "No public API was affected in the making of this changeset."
StandardDoclet is a public SPI. Doclets may extend that class, but it's the
"container" that calls the `getSupportedOptions` method. A corner case where the
client calls `getSupportedOptions` would be an implementation of Doclet that
delegates calls to an internal instance of StandardDoclet:
public class MyDoclet implements Doclet {
private final StandardDoclet standardDoclet = new StandardDoclet();
// ...
@Override
public Set<? extends Option> getSupportedOptions() {
Set<Option> supportedOptions = standardDoclet.getSupportedOptions();
supportedOptions.add(new MyOption()); // additional option
// ...
return supportedOptions;
}
private static class MyOption implements Doclet.Option {
// ...
}
// ...
}
Agreed, this is a somewhat contrived example made for the sake of the argument.
>> 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.
>
> The option names are often horrible and do not provide a really good
> precedent.
Agreed.
> It's tempting to an an informational source-only annotation that identifies
> the options that affect each field, but without any checking, such annotations
> would be little better than comments ... which is why I added comments
> to identify the options for each value.
This could be addressed another way. Instead of having two separate
abstractions,
options classes and option fields, we could use a single abstraction, types.
We could use some sort of a container [1]. The downside might be having more
types. A somewhat related design can be seen in java.net.SocketOption API [2].
That latter API tackles the need for more types by relying on option names, yet
still benefits from the type-safety.
That could allow for more collocation of the code related to command-line
options.
> <snip>
>
>> 6. AbstractMemberWriter's fields `printedSummaryHeader` and `nodepr` seem
>> not to
>> be used. Can those be deleted?
>
> Deleted
The `BaseOptions.docFileDestDirName` field doesn't seem to be accessed from
anywhere. Should it be deleted?
> <snip>
>
>> 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
> I fixed all instances found by searching for "command line opt"
Thanks!
The below is not a criticism of the refactoring, but rather a commentary on
the current state of affairs.
I don't like that command-line options are represented by public fields that can
be freely accessed from any part of the code. What I find especially unsettling
is that some of the fields of the BaseOptions and HtmlOptions are written from
the outside of those classes. These fields include:
BaseOptions.docEncoding,
BaseOptions.javafx,
HtmlOptions.charset
This field is written *only* from the outside of HtmlOptions:
HtmlOptions.createOverview
All that points out that Configuration and Options are coupled more tightly than
we may think.
-Pavel
--------------------------------------------------------------------------------
[1] Effective Java, Second Edition by Joshua Bloch,
Item 29: Consider typesafe heterogeneous containers
[2] See java.net.SocketOption, java.net.StandardSocketOptions,
java.net.Socket.supportedOptions, java.net.Socket.getOption,
java.net.Socket.setOption
>> 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/
