From 15215362dd18ecb01b329d41e0b072532cd7144b Mon Sep 17 00:00:00 2001 From: Eren Yeager <92114074+wty-Bryant@users.noreply.github.com> Date: Fri, 7 Apr 2023 16:04:32 -0400 Subject: [PATCH] Solve audit accessibility violation issue (#418) * First draft of DocumentationConverter smart wrapping * First draft of DocumentationConverter smart wrapping * Add changelog to smart wrapping commit * Squirrel out function of wrapping and writing service client docs * Squirrel out function of wrapping and writing service client docs * Modify changelog description * First draft of DocumentationConverter smart wrapping * First draft of DocumentationConverter smart wrapping * Add changelog to smart wrapping commit * Squirrel out function of wrapping and writing service client docs * Squirrel out function of wrapping and writing service client docs * Delete previous version's comment * Optimize DocumentaionConverter write wrapped text logic * modify changelog description * Optimize DocumentaionConverter write wrapped text logic * change remaining documentation converter and enum test code * change enum test code --------- Co-authored-by: Tianyi Wang --- .../1a8aa15f7f0649e3ba084e5ab48a6345.json | 8 ++ .../go/codegen/DocumentationConverter.java | 105 ++++++++++++++---- .../amazon/smithy/go/codegen/GoWriter.java | 4 +- .../codegen/DocumentationConverterTest.java | 56 +++++----- 4 files changed, 119 insertions(+), 54 deletions(-) create mode 100644 .changelog/1a8aa15f7f0649e3ba084e5ab48a6345.json diff --git a/.changelog/1a8aa15f7f0649e3ba084e5ab48a6345.json b/.changelog/1a8aa15f7f0649e3ba084e5ab48a6345.json new file mode 100644 index 000000000..32de68be1 --- /dev/null +++ b/.changelog/1a8aa15f7f0649e3ba084e5ab48a6345.json @@ -0,0 +1,8 @@ +{ + "id": "1a8aa15f-7f06-49e3-ba08-4e5ab48a6345", + "type": "bugfix", + "description": "correct list formatting in service client docs", + "modules": [ + "codegen" + ] +} \ No newline at end of file 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 dc84f17f3..dd5c9359b 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 @@ -63,7 +63,7 @@ private DocumentationConverter() {} * @param docs commonmark formatted documentation * @return godoc formatted documentation */ - public static String convert(String docs) { + public static String convert(String docs, int docWrapLength) { // Smithy's documentation format is commonmark, which can inline html. So here we convert // to html so we have a single known format to work with. String htmlDocs = HtmlRenderer.builder().escapeHtml(false).build().render(MARKDOWN_PARSER.parse(docs)); @@ -72,7 +72,7 @@ public static String convert(String docs) { htmlDocs = Jsoup.clean(htmlDocs, GODOC_ALLOWLIST); // Now we parse the html and visit the resultant nodes to render the godoc. - FormattingVisitor formatter = new FormattingVisitor(); + FormattingVisitor formatter = new FormattingVisitor(docWrapLength); Node body = Jsoup.parse(htmlDocs).body(); NodeTraversor.traverse(formatter, body); return formatter.toString(); @@ -80,7 +80,7 @@ public static String convert(String docs) { private static class FormattingVisitor implements NodeVisitor { private static final Set TEXT_BLOCK_NODES = SetUtils.of( - "br", "p", "h1", "h2", "h3", "h4", "h5", "h6" + "br", "p", "h1", "h2", "h3", "h4", "h5", "h6", "note" ); private static final Set LIST_BLOCK_NODES = SetUtils.of("ul", "ol"); private static final Set CODE_BLOCK_NODES = SetUtils.of("pre", "code"); @@ -88,12 +88,19 @@ private static class FormattingVisitor implements NodeVisitor { private boolean needsListPrefix = false; private boolean shouldStripPrefixWhitespace = false; + private int docWrapLength; + private int listDepth; + // the last line string written, used to calculate the remaining spaces to reach docWrapLength + // and determine if a split char is needed between it and next string + private String lastLineString; - FormattingVisitor() { + FormattingVisitor(int docWrapLength) { writer = new CodeWriter(); writer.trimTrailingSpaces(false); writer.trimBlankLines(); writer.insertTrailingNewline(false); + this.docWrapLength = docWrapLength; + this.lastLineString = ""; } @Override @@ -109,7 +116,7 @@ public void head(Node node, int depth) { writeNewline(); writeIndent(); } else if (LIST_BLOCK_NODES.contains(name)) { - writeNewline(); + listDepth++; } else if (name.equals("li")) { // We don't actually write out the list prefix here in case the list element // starts with one or more text blocks. By deferring writing those out until @@ -119,13 +126,6 @@ public void head(Node node, int depth) { } } - private void writeNewline() { - // While jsoup will strip out redundant whitespace, it will still leave some. If we - // start a new line then we want to make sure we don't keep any prefixing whitespace. - shouldStripPrefixWhitespace = true; - writer.write(""); - } - private void writeText(TextNode node) { if (node.isBlank()) { return; @@ -133,19 +133,74 @@ private void writeText(TextNode node) { // Docs can have valid $ characters that shouldn't run through formatters. String text = node.text().replace("$", "$$"); - if (shouldStripPrefixWhitespace) { shouldStripPrefixWhitespace = false; text = StringUtils.stripStart(text, " \t"); } - if (needsListPrefix) { - needsListPrefix = false; - writer.write(""); - writeIndent(); - text = "* " + StringUtils.stripStart(text, " \t"); + if (listDepth > 0) { + text = StringUtils.stripStart(text, " \t"); + if (needsListPrefix) { + needsListPrefix = false; + text = " - " + text; + writeNewline(); + } + + writeWrappedText(text, "\n "); + } else { + writeWrappedText(text, "\n"); + } + } + + private void writeWrappedText(String text, String newLineIndent) { + // check the last line's remaining space to see if test should be + // split to 2 parts to write to current and next line + // note that wrapped text will not contain desired indent at the beginning, + // so indent will be added to the wrapped text when it is written to a new line + + // right boundary index of text to be written to the same line exceeding + // neither docWrapLength nor text length + int trailingLineCutoff = Math.min(Math.max(docWrapLength - lastLineString.length(), 0), text.length()); + // the index of last space on the left of boundary if exist + int lastSpace = text.substring(0, trailingLineCutoff).lastIndexOf(" "); + // if current line is large enough to put the text, just append complete text to current line + // otherwise, cut out next line string starting from lastSpace index + String appendString = trailingLineCutoff < text.length() ? text.substring(0, lastSpace + 1) : text; + String nextLineString = trailingLineCutoff < text.length() ? text.substring(lastSpace + 1) : ""; + + if (!appendString.isEmpty()) { + ensureSplit(" ", appendString); + writeInline(appendString); + } + if (!nextLineString.isEmpty()) { + nextLineString = StringUtils.stripStart(newLineIndent, "\n") + + StringUtils.wrap(nextLineString, docWrapLength, newLineIndent, false); + writeNewline(); + writeInline(nextLineString); + } + } + + private void ensureSplit(String split, String text) { + if (!text.startsWith(split) && !lastLineString.isEmpty() && !lastLineString.endsWith(split)) { + writeInline(split); } - writer.writeInline(text); + } + + private void writeNewline() { + // While jsoup will strip out redundant whitespace, it will still leave some. If we + // start a new line then we want to make sure we don't keep any prefixing whitespace. + // need to refresh last line string + shouldStripPrefixWhitespace = true; + writer.write(""); + lastLineString = ""; + } + + private void writeInline(String contents, String... args) { + // write text at the current line, update last line string + String formatText = writer.format(contents, args); + writer.writeInlineWithNoFormatting(formatText); + formatText = lastLineString + formatText; + lastLineString = formatText.substring(formatText.lastIndexOf("\n") + 1); } void writeIndent() { @@ -223,21 +278,23 @@ public void tail(Node node, int depth) { writer.dedent(); } - if (TEXT_BLOCK_NODES.contains(name) || isTopLevelCodeBlock(node, depth) - || LIST_BLOCK_NODES.contains(name)) { - writeNewline(); + if (TEXT_BLOCK_NODES.contains(name) || isTopLevelCodeBlock(node, depth)) { 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. - writer.writeInline(" ($L)", url); + 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; - writer.write(""); } } diff --git a/codegen/smithy-go-codegen/src/main/java/software/amazon/smithy/go/codegen/GoWriter.java b/codegen/smithy-go-codegen/src/main/java/software/amazon/smithy/go/codegen/GoWriter.java index db941d2c7..d4640d51c 100644 --- a/codegen/smithy-go-codegen/src/main/java/software/amazon/smithy/go/codegen/GoWriter.java +++ b/codegen/smithy-go-codegen/src/main/java/software/amazon/smithy/go/codegen/GoWriter.java @@ -640,8 +640,8 @@ private void writeDocs(AbstractCodeWriter writer, Runnable runnable) { } private void writeDocs(AbstractCodeWriter writer, int docWrapLength, String docs) { - String wrappedDoc = StringUtils.wrap(DocumentationConverter.convert(docs), docWrapLength); - writeDocs(writer, () -> writer.write(wrappedDoc.replace("$", "$$"))); + String convertedDoc = DocumentationConverter.convert(docs, docWrapLength); + writeDocs(writer, () -> writer.write(convertedDoc.replace("$", "$$"))); } /** 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 5a10f47cb..da5833861 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 @@ -9,11 +9,12 @@ import org.junit.jupiter.params.provider.MethodSource; public class DocumentationConverterTest { + private static final int DEFAULT_DOC_WRAP_LENGTH = 80; @ParameterizedTest @MethodSource("cases") void convertsDocs(String given, String expected) { - assertThat(DocumentationConverter.convert(given), equalTo(expected)); + assertThat(DocumentationConverter.convert(given, DEFAULT_DOC_WRAP_LENGTH), equalTo(expected)); } private static Stream cases() { @@ -36,35 +37,35 @@ private static Stream cases() { ), Arguments.of( "
  • Testing 1 2 3
  • FooBar
", - "* Testing 1 2 3\n\n* FooBar" + " - Testing 1 2 3\n - FooBar" ), Arguments.of( "
  • Testing 1 2 3
  • FooBar
", - "* Testing 1 2 3\n\n* FooBar" + " - Testing 1 2 3\n - FooBar" ), Arguments.of( "
  • Testing 1 2 3
  • FooBar
", - "* Testing 1 2 3\n\n* FooBar" + " - Testing 1 2 3\n - FooBar" ), Arguments.of( "

  • Testing 1 2 3

  • FooBar

", - "* Testing 1 2 3\n\n* FooBar" + " - Testing 1 2 3\n - FooBar" ), Arguments.of( "
  • Testing: 1 2 3
  • FooBar
", - "* Testing: 1 2 3\n\n* FooBar" + " - Testing : 1 2 3\n - FooBar" ), Arguments.of( "

  • FOO Bar

  • Xyz ABC

", - "* FOO Bar\n\n* Xyz ABC" + " - FOO Bar\n - Xyz ABC" ), Arguments.of( "
  • foo
  • \tbar
  • \nbaz
", - "* foo\n\n* bar\n\n* baz" + " - foo\n - bar\n - baz" ), Arguments.of( "

Testing: 1 2 3

", - "Testing: 1 2 3" + "Testing : 1 2 3" ), Arguments.of( "
Testing
", @@ -89,14 +90,13 @@ 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 replication " - + "configuration, see Cross-Region Replication (CRR) " - + "(https://docs.aws.amazon.com/AmazonS3/latest/dev/crr.html) 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." ), Arguments.of( "* foo\n* bar", - "* foo\n\n* bar" + " - foo\n - bar" ), Arguments.of( "[a link](https://example.com)", @@ -104,21 +104,21 @@ private static Stream cases() { ), Arguments.of("", ""), Arguments.of("", ""), - 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("# 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("Inline `code`", "Inline code"), - Arguments.of("```\ncode block\n```", " code block"), - Arguments.of("```java\ncode block\n```", " code block"), + 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") );