Yeah, I was very aware of the weirdness when I submitted the review,
particularly for the names that are more like verbs than nouns or
adjectives.
My short-term justification is, as Hannes noted, they're always invoked
on an "options" object. Further out, I'm open to better naming. One
thing to note is that this changeset moves us towards a more consistent
design that is more amenable to IDE-based updates (i.e. "rename method"
refactoring) down the line.
We can also try and come up with a scheme for naming option accessor
methods that covers both tool options and all the doclet options.
-- Jon
On 1/27/20 7:22 AM, Hannes Wallnöfer wrote:
I forgot: +1 on the change itself.
Hannes
Am 27.01.2020 um 16:20 schrieb Hannes Wallnöfer <[email protected]>:
I agree some of these method names taken on their own are slightly deceptive.
But given their mostly ()boolean signatures and the fact that they’re always
invoked on ‚options‘ or similar make their purpose clear enough without having
to look at their implementation or doc comments.
Given that neither get*, is*, or has* prefix is really great for this use case
I think keeping these names and maybe adding a comment as suggested by Pavel is
the best option.
Hannes
Am 27.01.2020 um 14:38 schrieb Pavel Rappo <[email protected]>:
I remember we had an offline conversation on whether we should use shorter
method names for accessing stuff [ stuff() ], or more familiar ones
[ getStuff()/hasStuff()/isStuff()/etc. ].
I remember saying that I had no strong opinion about it. Not thank I'm
rethinking it, but I can now see how some of the accessor methods look weird.
Unsurprisingly, those are the ones whose name starts with a verb. For example,
copyDocfileSubdirs(),
createIndex(),
createOverview(),
linkSource(),
showAuthor(),
showVersion(),
splitIndex(),
summarizeOverriddenMethods(), etc.
Naming is hard, no doubt about it. While giving something a name, that name
should not be expected to be the sole guide on how to use that something.
A name should be a good mnemonic.
We could put a top-level comment in both BaseOptions and HtmlOptions. Something
to the effect that "unless otherwise stated, the methods below do not have any
behavior beyond simply accessing stuff".
Otherwise, looks good.
-Pavel
On 25 Jan 2020, at 00:33, Jonathan Gibbons <[email protected]> wrote:
Please review a conceptually simple review to encapsulate the fields holding
the values of doclet options, in the BaseOptions and HtmlOptions classes.
The encapsulation was done with an IDE. The fields are all changed to private
access; the access methods are package-private by default, and public when
necessary, although in general, there is no problem if it is determined they
should all have public access. This is the same policy as was done for
ToolOptions, currently in review.
Some fields need public "setters". These were added manually.
In a few places, code was simplified manually, joining short lines, or
replacing `configuration.getOptions()` with just `options`.
This is intended to be the last in the current round of option-code cleanup,
for now, although I think there is scope for future work to rationalize the
names of the fields and corresponding accessor methods.
-- Jon
JBS: https://bugs.openjdk.java.net/browse/JDK-8237845
Webrev: http://cr.openjdk.java.net/~jjg/8237845/webrev.00/
