On Fri, 15 Nov 2024 17:25:59 GMT, Nizar Benalla <nbena...@openjdk.org> wrote:
>> Please review this patch to render javadocs as `<K, V, T>` rather than >> `<K,V,T>` to match naming conventions and make the rendered output slighly >> nicer to read. >> >> Passes langtool tests. >> >> The [generated >> docs](https://cr.openjdk.org/~nbenalla/GeneratedDocs/K-V-space/docs/api/index.html) >> only include `java.base`. >> >> Note for reviewers: >> >> In `TestInheritence`, B is a user defined class and `TypeMirror::getKind` >> returns `DECLARED `. Which why we see this output. >> >> >> >> html >> >> Class D<R,S> >> java.lang.Object >> [pkg.A](https://htmledit.squarefree.com/A.html)<S, >> [B](https://htmledit.squarefree.com/B.html)> >> [pkg.B](https://htmledit.squarefree.com/B.html)<S, >> [B](https://htmledit.squarefree.com/B.html)> >> pkg.D<R,S> > > Nizar Benalla has updated the pull request with a new target base due to a > merge or a rebase. The incremental webrev excludes the unrelated changes > brought in by the merge/rebase. The pull request contains three additional > commits since the last revision: > > - small improvement after getting review comments. > > Update tests > - Merge remote-tracking branch 'upstream/master' into javadoc-whitespace > - Add small whitespace before map parameters src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/HtmlLinkFactory.java line 412: > 410: } else { > 411: links.add(",").add(HtmlTree.WBR()); > 412: } Overall, I think this is a smart solution that works well for the vast majority of cases. One issue I see is that bounded type parameters can also get very complext and benefit from a space after the comma as well. Examples for this are [interface Spliterator.OfPrimitive][1] and [this method in HttpResponse.BodySubscribers][2]. [1]: https://docs.oracle.com/en/java/javase/23/docs/api/java.base/java/util/Spliterator.OfPrimitive.html [2]: https://docs.oracle.com/en/java/javase/23/docs/api/java.net.http/java/net/http/HttpResponse.BodySubscribers.html#fromSubscriber(java.util.concurrent.Flow.Subscriber,java.util.function.Function) The type parameters in the method signature currently get the`Text.NL` inserted below, which is rendered as newline for long type parameters and space otherwise. This is a bit of a hack (and was also one of the reasons that got us started with this), and should be replaced and handled by the new code (preferably by only adding a newline if it is actually needed because there is a bounded type parameter). So I think the full solution would be to also add a space for bounded type parameters if the bounds are being displayed in the signature. Unfortunately, this makes the solution more complicated and less elegant. Basically we would have to do the checks in lines 178-190 of this class. Maybe the pragmatic solution would be to render all parts into a list and check their length? ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/21759#discussion_r1846996325
