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\n - FooBar"
+ " - Testing 1 2 3\n\n - FooBar"
),
Arguments.of(
"",
@@ -57,7 +53,7 @@ private static Stream cases() {
),
Arguments.of(
"",
- " - FOO Bar\n - Xyz ABC"
+ " - FOO Bar\n\n - Xyz ABC"
),
Arguments.of(
"",
@@ -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")
);
}
}