RFR: [15] JDK-8237492 Reorganize impl of doclet options
Jonathan Gibbons
jonathan.gibbons at oracle.com
Thu Jan 23 19:05:33 UTC 2020
Kumar,
Thanks for the feedback; I'll incorporate this change. (I'm currently
waiting on a minor CSR approval).
-- Jon
On 01/23/2020 09:47 AM, Kumar Srinivasan wrote:
> Hi Jon,
>
> Sorry for the late arrival, I did not do a deep dive, however this
> caught my eye.
> I realize you must have used the IDE to refactor, in any case, in the
> lines you have changed there
> are these old constructs:
>
> src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/AbstractOverviewIndexWriter.java
> + if (doctitle.length() > 0) {
>
> These can be replaced with doctitle.isEmpty().
>
>
>> On Jan 22, 2020, at 11:29 AM, Jonathan Gibbons
>> <jonathan.gibbons at oracle.com <mailto:jonathan.gibbons at oracle.com>> wrote:
>>
>> Pavel,
>>
>> Thanks for your additional feedback. There's a couple of actionable
>> items, most notably
>> with respect to StandardDoclet.
>>
>> As for the rest, I agree there's a whole bunch more stuff that we
>> *could* do, but I would
>> prefer to get the work so far staged into the repo. As the ancient
>> Roman's used to say,
>>
>> javadoc was not cleaned up in a single changeset.
>
> Very true indeed!. :)
>
>
> Kumar
>
>>
>> -- Jon
>>
>>
>> On 01/22/2020 08:00 AM, Pavel Rappo wrote:
>>>> On 21 Jan 2020, at 18:55, Jonathan Gibbons
>>>> <jonathan.gibbons at oracle.com <mailto:jonathan.gibbons at oracle.com>>
>>>> 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.
>>>>
>>>> https://nam04.safelinks.protection.outlook.com/?url=http:%2F%2Fcr.openjdk.java.net%2F~jjg%2F8237492%2Fwebrev.01%2F&data=02%7C01%7Ckusrinivasan%40vmware.com%7Ccb446ac59a204a6741f608d79f71c5fe%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C0%7C0%7C637153183321632928&sdata=Tio9bquIDiwfRh4R8bmfrMqxShy8JP5KlW%2FdmBeQv60%3D&reserved=0
>>>> <https://nam04.safelinks.protection.outlook.com/?url=http:%2F%2Fcr.openjdk.java.net%2F%7Ejjg%2F8237492%2Fwebrev.01%2F&data=02%7C01%7Ckusrinivasan%40vmware.com%7Ccb446ac59a204a6741f608d79f71c5fe%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C0%7C0%7C637153183321632928&sdata=Tio9bquIDiwfRh4R8bmfrMqxShy8JP5KlW%2FdmBeQv60%3D&reserved=0>
>>> 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.
>>
>> You're right; I'd missed that this was a change to StandardDoclet,
>> which is a public API.
>> This will need to be sorted out, separately.
>>
>>>
>>>>> 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.
>>
>> I think this is more than I want to consider for this round of cleanup.
>>
>>>
>>>> <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?
>>
>> Yes, will do.
>>
>>>
>>>> <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
>>
>> I agree that the mutable public fields are not great, and that we
>> might want to use
>> an IDE to encapsulate the fields. The 3 fields that are modified
>> from inside and out of
>> the class have default values being set. If/when we encapsulate the
>> fields, it may be
>> possible to handle the fault in the access method.
>>
>> The same probably goes for HtmlOptions.createOverview.
>>
>>>
>>> All that points out that Configuration and Options are coupled more
>>> tightly than
>>> we may think.
>>
>> No, it just means we have not quite got the boundary in quite the
>> right place yet.
>> But, they are clearly related since part of the configuration is the
>> options, but there
>> is more code in the configuration classes that uses the options to
>> determine more
>> configuration values.
>>
>>>
>>> -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
>>>>>> <jonathan.gibbons at oracle.com
>>>>>> <mailto:jonathan.gibbons at oracle.com>> 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://nam04.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugs.openjdk.java.net%2Fbrowse%2FJDK-8237492&data=02%7C01%7Ckusrinivasan%40vmware.com%7Ccb446ac59a204a6741f608d79f71c5fe%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C0%7C0%7C637153183321632928&sdata=w2xJGSbuItjHTHfg95KhpWbtsBOSgR02JoLoxwHGPvM%3D&reserved=0
>>>>>> Webrev:
>>>>>> https://nam04.safelinks.protection.outlook.com/?url=http:%2F%2Fcr.openjdk.java.net%2F~jjg%2F8237492%2Fwebrev%2F&data=02%7C01%7Ckusrinivasan%40vmware.com%7Ccb446ac59a204a6741f608d79f71c5fe%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C0%7C0%7C637153183321632928&sdata=bcgajooeHWoA5vUjrjEsmImYBOcdf21Y3o49jg7tdPc%3D&reserved=0
>>>>>> <https://nam04.safelinks.protection.outlook.com/?url=http:%2F%2Fcr.openjdk.java.net%2F%7Ejjg%2F8237492%2Fwebrev%2F&data=02%7C01%7Ckusrinivasan%40vmware.com%7Ccb446ac59a204a6741f608d79f71c5fe%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C0%7C0%7C637153183321632928&sdata=bcgajooeHWoA5vUjrjEsmImYBOcdf21Y3o49jg7tdPc%3D&reserved=0>
>>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.java.net/pipermail/javadoc-dev/attachments/20200123/7a015da5/attachment-0001.htm>
More information about the javadoc-dev
mailing list