On Wed, 16 Mar 2022 16:32:09 GMT, Pavel Rappo <[email protected]> wrote:
> This PR started in a draft mode as a refactoring effort. After a few commits
> and chats with Jonathan Gibbons, I decided that further refactoring belongs
> to follow-up PRs and that this PR could be marked as "Ready for review".
>
> The motivation for the initial refactoring effort was as follows. The word
> "tree" is heavily overloaded in the JavaDoc codebase, both in comments and
> code. Here are some of the contexts this word appears in:
>
> * Hierarchy Tree (i.e. overview-tree.html and package-tree.html)
> * DocTree (AST nodes)
> * ClassTree, *TreeWriter (data structures and entities related to
> supertype-subtype relationship)
> * HtmlTree (HTML nodes) and specifically UL/OL elements which are nested
> lists
>
> Sometimes contexts overlap, making the word "tree" ambiguous. To reduce this
> ambiguity, the word "tree" should be dropped in favor of a more specific word
> of phrase where possible.
>
> In the case where the context is
> `jdk.javadoc.internal.doclets.toolkit.Content`, the programmer is already
> aware that they are dealing with trees in the sense that `Content` objects
> support recursive composition. There's no need to have the phrase "content
> tree" where "content" would do. Moreover, in the context that is exclusively
> about `Content` objects, the word "content" can be dropped too, since the
> type information is assumed.
>
> As an example of content overlap, have a look at the source of
> `jdk.javadoc.internal.doclets.toolkit.builders.MemberSummaryBuilder::buildSummary`.
> This method used to needlessly mix DocTree with Content tree.
OK ...
Well, there's a huge amount of work here, most of which is a genuine
improvement, so well done for all that, but I get the sense this work got out
of control a bit, given there are many "themes" in this work, each of which
would have been a worthy and significant PR in its own right. As a result, the
whole is much harder to review than the sum of its parts.
The primary theme seems to be to remove the word `tree` from method names and
comments when it is either in appropriate or somewhat unnecessary.
Amongst the additional themes that are not directly related to the JBS issue:
* use of `var`
* removing unnecessary comments from overriding methods
* use of `{@return}`
* minor grammar
Don't get me wrong: these are all good improvements. It's just that doing them
all together makes it harder to see the trees in the forest. (sic)
You are somewhat fortunate in that there is not a lot of major concurrent
development in javadoc right now; the merge could have been horrible!
So, to summarize, thanks for taking all this on; I'll approve it, but please
file JBS issues for any follow-up cleanup that you consider still needs to be
done.
src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/AbstractExecutableMemberWriter.java
line 268:
> 266: *
> 267: * @param member the member to get the exception information for
> 268: * @return the exception information
For later ... use `Returns`
Also, maybe better grammar
@param member the member for which to get/obtain/ the exception information
src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/AbstractMemberWriter.java
line 190:
> 188: * @param target the content to which the modifiers and type will be
> added
> 189: */
> 190: protected void addModifierAndType(Element member, TypeMirror type,
For later: maybe adjust method name as well
src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/AbstractOverviewIndexWriter.java
line 134:
> 132: String doctitle = configuration.getOptions().docTitle();
> 133: if (!doctitle.isEmpty()) {
> 134: RawHtml title = new RawHtml(doctitle);
why not `var`?
src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/ConstantsSummaryWriterImpl.java
line 71:
> 69:
> 70: /**
> 71: * The HTML node for constant values summary currently being written.
This change seems unnecessary and just in produces an unnecessary synonym
src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/Contents.java
line 346:
> 344: *
> 345: * @param key the key for the desired string
> 346: * @return the string
No, that suggests it returns the String itself
Suggest `content containing the string`
src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/Contents.java
line 359:
> 357: * @param key the key for the desired string
> 358: * @param o0 string or content argument to be formatted into the
> result
> 359: * @return the string
see previous
src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/Contents.java
line 373:
> 371: * @param o0 string or content argument to be formatted into the
> result
> 372: * @param o1 string or content argument to be formatted into the
> result
> 373: * @return the string
see previous
src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/Contents.java
line 388:
> 386: * @param o1 string or content argument to be formatted into the
> result
> 387: * @param o2 string or content argument to be formatted into the
> result
> 388: * @return the string
see previous
src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/Contents.java
line 449:
> 447: *
> 448: * @param key the key for the desired string
> 449: * @return the string
see similar
src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/Contents.java
line 461:
> 459: *
> 460: * @param text the string
> 461: * @return the string content
interesting/inconsistent ;-)
src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/HtmlDocletWriter.java
line 321:
> 319: * Adds the tags information.
> 320: *
> 321: * @param e the Element for which the tags will be generated
`Element` should be `element`
src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/HtmlDocletWriter.java
line 1906:
> 1904:
> 1905: /**
> 1906: * {@return the annotation types info for the given element}
I would be surprised if this returns content containing the annotation types,
as compared to the annotations themselves
src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/PackageTreeWriter.java
line 93:
> 91:
> 92: /**
> 93: * Generate a separate tree file.
Generates
src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/PackageWriterImpl.java
line 80:
> 78:
> 79: /**
> 80: * The HTML element for the section tag being written.
Hmm, it wasn't wrong to use `tree` here; the goal should not be to remove as
many uses of the word `tree` as possible! Future developers will wonder, why
does the code avoid all use of the word `tree`
src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/Signatures.java
line 393:
> 391: * Set the return type for an executable member.
> 392: *
> 393: * @param returnType the return type to add.
remove period
src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/Signatures.java
line 415:
> 413: * Set the parameter information of an executable member.
> 414: *
> 415: * @param content the parameter information.
maybe a general cleanup later on fixing trailing periods?
src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/Signatures.java
line 435:
> 433:
> 434: /**
> 435: * Set the annotation information of a member.
Sets
src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/TreeWriter.java
line 166:
> 164: String title =
> resources.getText("doclet.Window_Class_Hierarchy");
> 165: HtmlTree bodyTree = getBody(getWindowTitle(title));
> 166: bodyContents.setHeader(getHeader(PageMode.TREE)); // idempotent
why is the comment significant?
src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/markup/Script.java
line 102:
> 100:
> 101: /**
> 102: * Returns a "live" view of the script as a {@code Content} object.
Don't change it now, but the comment abut a "live view" is sort-of unnecessary
because in general most HTMLTree objects are mutable.
src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/ModuleSummaryWriter.java
line 64:
> 62:
> 63: /**
> 64: * Wrap the content into summary section.
Wraps
src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/SerializedFormWriter.java
line 71:
> 69:
> 70: /**
> 71: * Add the serialized package to the serialized summaries.
Adds
-------------
Marked as reviewed by jjg (Reviewer).
PR: https://git.openjdk.java.net/jdk/pull/7843
