From fb3ea81aa3fb6de616f9ab4bdd24362a1e84e185 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Luis=20Madrigal=20=F0=9F=90=A7?= Date: Tue, 7 May 2024 10:18:09 -0400 Subject: [PATCH] fix: improve Go doc formatter Improvements: - Use Go doc links introduced in Go 1.19 - Separate paragraphs so they are rendered as separate blocks of text fixes aws/aws-sdk-go-v2#2597 --- codegen/build.gradle.kts | 2 +- .../go/codegen/DocumentationConverter.java | 60 +++++++++++++++---- .../enum-shape-test/expected/types/enums.go | 5 +- .../expected/types/enums.go | 5 +- .../codegen/DocumentationConverterTest.java | 46 +++++++------- 5 files changed, 79 insertions(+), 39 deletions(-) diff --git a/codegen/build.gradle.kts b/codegen/build.gradle.kts index ab3afcdce..8fb1fab64 100644 --- a/codegen/build.gradle.kts +++ b/codegen/build.gradle.kts @@ -90,7 +90,7 @@ subprojects { dependencies { testImplementation("org.junit.jupiter:junit-jupiter-api:5.4.0") testImplementation("org.junit.jupiter:junit-jupiter-engine:5.4.0") - testCompileOnly("org.junit.jupiter:junit-jupiter-params:5.4.0") + testImplementation("org.junit.jupiter:junit-jupiter-params:5.4.0") testImplementation("org.hamcrest:hamcrest:2.1") } diff --git a/codegen/smithy-go-codegen/src/main/java/software/amazon/smithy/go/codegen/DocumentationConverter.java b/codegen/smithy-go-codegen/src/main/java/software/amazon/smithy/go/codegen/DocumentationConverter.java index dd5c9359b..bc69ab8d0 100644 --- a/codegen/smithy-go-codegen/src/main/java/software/amazon/smithy/go/codegen/DocumentationConverter.java +++ b/codegen/smithy-go-codegen/src/main/java/software/amazon/smithy/go/codegen/DocumentationConverter.java @@ -15,7 +15,10 @@ package software.amazon.smithy.go.codegen; +import java.util.HashMap; +import java.util.Map; import java.util.Set; +import java.util.stream.Collectors; import org.commonmark.node.BlockQuote; import org.commonmark.node.FencedCodeBlock; import org.commonmark.node.Heading; @@ -25,6 +28,7 @@ import org.commonmark.parser.Parser; import org.commonmark.renderer.html.HtmlRenderer; import org.jsoup.Jsoup; +import org.jsoup.nodes.Element; import org.jsoup.nodes.Node; import org.jsoup.nodes.TextNode; import org.jsoup.safety.Safelist; @@ -42,9 +46,9 @@ public final class DocumentationConverter { // This allowlist strips out anything we can't reasonably convert, vastly simplifying the // node tree we end up having to crawl through. private static final Safelist GODOC_ALLOWLIST = new Safelist() - .addTags("code", "pre", "ul", "ol", "li", "a", "br", "h1", "h2", "h3", "h4", "h5", "h6") + .addTags("code", "pre", "ul", "ol", "li", "a", "br", "h1", "h2", "h3", "h4", "h5", "h6", "p") .addAttributes("a", "href") - .addProtocols("a", "href", "http", "https", "mailto"); + .addProtocols("a", "href", "http", "https"); // Construct a markdown parser that specifically ignores parsing indented code blocks. This // is because HTML blocks can have really wonky formatting that can be mis-attributed to an @@ -85,6 +89,8 @@ private static class FormattingVisitor implements NodeVisitor { private static final Set LIST_BLOCK_NODES = SetUtils.of("ul", "ol"); private static final Set CODE_BLOCK_NODES = SetUtils.of("pre", "code"); private final CodeWriter writer; + // Godoc links are added at the end as `[text]: http://example.com` so keeping a collection to add them later + private final Map links; private boolean needsListPrefix = false; private boolean shouldStripPrefixWhitespace = false; @@ -100,6 +106,7 @@ private static class FormattingVisitor implements NodeVisitor { writer.trimBlankLines(); writer.insertTrailingNewline(false); this.docWrapLength = docWrapLength; + this.links = new HashMap<>(); this.lastLineString = ""; } @@ -110,9 +117,31 @@ public void head(Node node, int depth) { writer.indent(); } + if (node.nodeName().equals("a")) { + // Logic to format anchors as Go Links https://tip.golang.org/doc/comment#links + Element element = (Element) node; + String text = element.text(); + String url = element.absUrl("href"); + if (url.isEmpty()) { + // an empty anchor won't have a reference at the end, so just + // output it directly to the output + writer.writeInlineWithNoFormatting(text); + return; + } + String wrappedAnchorText = "[" + text + "]"; + writer.writeInlineWithNoFormatting(wrappedAnchorText); + links.put(text, url); + } + + Node parentNode = node.parentNode(); + if (parentNode != null && parentNode.nodeName().equals("a") && node instanceof TextNode) { + // anchor tags get processed twice: once as anchor tags and another one as + // textNodes. Since this was already processed as anchor, no need to do anything else + return; + } if (node instanceof TextNode) { writeText((TextNode) node); - } else if (TEXT_BLOCK_NODES.contains(name) || isTopLevelCodeBlock(node, depth)) { + } else if (isTopLevelCodeBlock(node, depth)) { writeNewline(); writeIndent(); } else if (LIST_BLOCK_NODES.contains(name)) { @@ -279,19 +308,16 @@ public void tail(Node node, int depth) { } if (TEXT_BLOCK_NODES.contains(name) || isTopLevelCodeBlock(node, depth)) { + // A single newline is just treated as a line break, but gets + // rendered as the same block. + // Two ensure the text gets treated as a separate text block + writeNewline(); writeNewline(); } else if (LIST_BLOCK_NODES.contains(name)) { listDepth--; if (listDepth == 0) { writeNewline(); } - } else if (name.equals("a")) { - String url = node.absUrl("href"); - if (!url.isEmpty()) { - // godoc can't render links with text bodies, so we simply append the link. - // Full links do get rendered. - writeInline(" ($L)", url); - } } else if (name.equals("li")) { // Clear out the expectation of a list element if the element's body is empty. needsListPrefix = false; @@ -313,8 +339,22 @@ public String toString() { } result = String.join("\n", lines); + // iterate over every link found and append it at the end + String allLinks = links.entrySet().stream() + .map(link -> formatLink(link)) + .collect(Collectors.joining("\n")); + if (!allLinks.isEmpty()) { + result += "\n\n" + allLinks; + } + // Strip out leading and trailing newlines. return StringUtils.strip(result, "\n"); } + + private String formatLink(Map.Entry link) { + String anchor = link.getKey(); + String destination = link.getValue(); + return "[%s]: %s".formatted(anchor, destination); + } } } diff --git a/codegen/smithy-go-codegen/src/main/resources/software/amazon/smithy/go/codegen/smithy-tests/enum-shape-test/expected/types/enums.go b/codegen/smithy-go-codegen/src/main/resources/software/amazon/smithy/go/codegen/smithy-tests/enum-shape-test/expected/types/enums.go index a0d715126..ca289948c 100644 --- a/codegen/smithy-go-codegen/src/main/resources/software/amazon/smithy/go/codegen/smithy-tests/enum-shape-test/expected/types/enums.go +++ b/codegen/smithy-go-codegen/src/main/resources/software/amazon/smithy/go/codegen/smithy-tests/enum-shape-test/expected/types/enums.go @@ -14,8 +14,9 @@ const ( ) // Values returns all known values for Suit. Note that this can be expanded in the -// future, and so it is only as up to date as the client. The ordering of this -// slice is not guaranteed to be stable across updates. +// future, and so it is only as up to date as the client. +// +// The ordering of this slice is not guaranteed to be stable across updates. func (Suit) Values() []Suit { return []Suit{ "DIAMOND", diff --git a/codegen/smithy-go-codegen/src/main/resources/software/amazon/smithy/go/codegen/smithy-tests/int-enum-shape-test/expected/types/enums.go b/codegen/smithy-go-codegen/src/main/resources/software/amazon/smithy/go/codegen/smithy-tests/int-enum-shape-test/expected/types/enums.go index ea165773b..209750e74 100644 --- a/codegen/smithy-go-codegen/src/main/resources/software/amazon/smithy/go/codegen/smithy-tests/int-enum-shape-test/expected/types/enums.go +++ b/codegen/smithy-go-codegen/src/main/resources/software/amazon/smithy/go/codegen/smithy-tests/int-enum-shape-test/expected/types/enums.go @@ -33,8 +33,9 @@ const ( ) // Values returns all known values for Suit. Note that this can be expanded in the -// future, and so it is only as up to date as the client. The ordering of this -// slice is not guaranteed to be stable across updates. +// future, and so it is only as up to date as the client. +// +// The ordering of this slice is not guaranteed to be stable across updates. func (Suit) Values() []Suit { return []Suit{ "DIAMOND", diff --git a/codegen/smithy-go-codegen/src/test/java/software/amazon/smithy/go/codegen/DocumentationConverterTest.java b/codegen/smithy-go-codegen/src/test/java/software/amazon/smithy/go/codegen/DocumentationConverterTest.java index da5833861..73583a14c 100644 --- a/codegen/smithy-go-codegen/src/test/java/software/amazon/smithy/go/codegen/DocumentationConverterTest.java +++ b/codegen/smithy-go-codegen/src/test/java/software/amazon/smithy/go/codegen/DocumentationConverterTest.java @@ -25,11 +25,7 @@ private static Stream cases() { ), Arguments.of( "a link", - "a link (https://example.com)" - ), - Arguments.of( - "a link", - "a link (https://example.com)" + "[a link]\n\n[a link]: https://example.com" ), Arguments.of( "empty link", @@ -49,7 +45,7 @@ private static Stream cases() { ), Arguments.of( "

  • Testing 1 2 3

  • FooBar

", - " - Testing 1 2 3\n - FooBar" + " - Testing 1 2 3\n\n - FooBar" ), Arguments.of( "
  • Testing: 1 2 3
  • FooBar
", @@ -57,7 +53,7 @@ private static Stream cases() { ), Arguments.of( "

  • FOO Bar

  • Xyz ABC

", - " - FOO Bar\n - Xyz ABC" + " - FOO Bar\n\n - Xyz ABC" ), Arguments.of( "
  • foo
  • \tbar
  • \nbaz
", @@ -90,9 +86,9 @@ private static Stream cases() { + " configuration, see " + "" + "Cross-Region Replication (CRR) in the Amazon S3 Developer Guide.

", - "Deletes the replication configuration from the bucket. For information about\n" + - "replication configuration, see Cross-Region Replication (CRR) (https://docs.aws.amazon.com/AmazonS3/latest/dev/crr.html)\n" + - "in the Amazon S3 Developer Guide." + " Deletes the replication configuration from the bucket. For information about\n" + + "replication configuration, see [Cross-Region Replication (CRR)]in the Amazon S3 Developer Guide.\n\n" + + "[Cross-Region Replication (CRR)]: https://docs.aws.amazon.com/AmazonS3/latest/dev/crr.html" ), Arguments.of( "* foo\n* bar", @@ -100,27 +96,29 @@ private static Stream cases() { ), Arguments.of( "[a link](https://example.com)", - "a link (https://example.com)" + "[a link]\n\n[a link]: https://example.com" ), Arguments.of("", ""), Arguments.of("", ""), - Arguments.of("# Foo\nbar", "Foo\nbar"), - Arguments.of("

Foo

bar", "Foo\nbar"), - Arguments.of("## Foo\nbar", "Foo\nbar"), - Arguments.of("

Foo

bar", "Foo\nbar"), - Arguments.of("### Foo\nbar", "Foo\nbar"), - Arguments.of("

Foo

bar", "Foo\nbar"), - Arguments.of("#### Foo\nbar", "Foo\nbar"), - Arguments.of("

Foo

bar", "Foo\nbar"), - Arguments.of("##### Foo\nbar", "Foo\nbar"), - Arguments.of("
Foo
bar", "Foo\nbar"), - Arguments.of("###### Foo\nbar", "Foo\nbar"), - Arguments.of("
Foo
bar", "Foo\nbar"), + Arguments.of("# Foo\nbar", "Foo\n\nbar"), + Arguments.of("

Foo

bar", "Foo\n\nbar"), + Arguments.of("## Foo\nbar", "Foo\n\nbar"), + Arguments.of("

Foo

bar", "Foo\n\nbar"), + Arguments.of("### Foo\nbar", "Foo\n\nbar"), + Arguments.of("

Foo

bar", "Foo\n\nbar"), + Arguments.of("#### Foo\nbar", "Foo\n\nbar"), + Arguments.of("

Foo

bar", "Foo\n\nbar"), + Arguments.of("##### Foo\nbar", "Foo\n\nbar"), + Arguments.of("
Foo
bar", "Foo\n\nbar"), + Arguments.of("###### Foo\nbar", "Foo\n\nbar"), + Arguments.of("
Foo
bar", "Foo\n\nbar"), Arguments.of("Inline `code`", "Inline code"), Arguments.of("```\ncode block\n ```", " code block"), Arguments.of("```java\ncode block\n ```", " code block"), Arguments.of("foo
bar", "foo\n\nbar"), - Arguments.of("

foo

", "foo") + Arguments.of("

foo

", "foo"), + Arguments.of("

paragraph one

paragraph two

", "paragraph one\n\nparagraph two"), + Arguments.of("someone", "someone") ); } }