fix: format method types and table in Client Overview #2361
Conversation
|
|
| for (MethodAndVariants method : methodAndVariantsList) { | ||
| tableBuilder | ||
| .append(" <tr>\n") | ||
| .append(" <td>") | ||
| .append(method.method) | ||
| .append(CommentFormatter.formatAsJavaDocComment(method.method, null)) |
There was a problem hiding this comment.
Let's see if we can refactor CommentFormatter so that it can be used for the whole table, otherwise this issue may happen again in the future. That being said, if it turns out to be more complicated that it supposed to, we can merge this for now and create another issue to track the refactoring effort.
There was a problem hiding this comment.
I investigated this a bit and I think it would be a little tricky to refactor CommentFormatter to be able to be applied to a whole table as I think it would need to have changes (or additions) to JavadocComment to only escape certain characters if they're not part of an html tag.
I could rewrite the table in general to use JavadocComment instead of StringBuilder so at least it would be explicitly clear when we are not applying standard formatting (e.g. escaping HTML characters). Let me know if you think that's worth doing or if I should create an issue for the larger refactoring effort.
There was a problem hiding this comment.
Tracking issue for refactor: #2385
Are you thinking something like the downstream tests we configured in java-shared-config: https://github.com/googleapis/java-shared-config/pull/662/files and applied to sdk-platform-java? |
|
I didn't think of the downstream testing specifically. Yes, a check similar to the downstream check will address my ask. There are various ways to solve that. I'm asking "Next time when somebody breaks the javadoc generation logic, please block the pull request." |
Cool. I created an issue #2380 to track that work as separate from this PR. |
Fixes #2366 ☕️
Confirmed with java-logging that running
mvn javadoc:aggregatesuccessfully builds.