Skip to content

Commit

Permalink
Fix formatter to correctly convert invalid doc comments (#2277)
Browse files Browse the repository at this point in the history
- Fixes an issue where a doc-comment above a trait-statement and member may get converted to a line-comment
- Updates the expected formatting of test case file (.formatted.smithy)
- Updates tests to use assertEquals, since it provides diff functionality in IDEs

---------

Co-authored-by: Miles Ziemer <mziemer@amazon.com>
  • Loading branch information
haydenbaker and milesziemer authored May 8, 2024
1 parent 0f2f274 commit e71e5c8
Show file tree
Hide file tree
Showing 4 changed files with 45 additions and 15 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,11 @@ public TokenTree apply(TokenTree tree) {
updateNestedChildren(cursor);
}

// Doc comments should not appear in TRAIT_STATEMENTS.
for (TreeCursor cursor : shapeSection.findChildrenByType(TreeType.TRAIT_STATEMENTS)) {
updateNestedChildren(cursor);
}

// Fix doc comments that come before apply statements.
TreeCursor shapeStatements = shapeSection.getFirstChild(TreeType.SHAPE_STATEMENTS);
if (shapeStatements != null) {
Expand All @@ -77,11 +82,15 @@ public TokenTree apply(TokenTree tree) {
updateNestedChildren(br);
}
}

// Remove trailing doc comments in member bodies.
// Fix any trailing doc comments in shape bodies
for (TreeCursor members : shapeStatements.findChildrenByType(TreeType.SHAPE_MEMBERS)) {
TreeCursor ws = members.getLastChild(TreeType.WS);
updateDirectChildren(ws);
TreeCursor closeBrace = members.getLastChild();
if (closeBrace != null) {
TreeCursor possibleTrailingWs = closeBrace.getPreviousSibling();
if (possibleTrailingWs != null && possibleTrailingWs.getTree().getType() == TreeType.WS) {
updateDirectChildren(possibleTrailingWs);
}
}
}
}

Expand Down
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
package software.amazon.smithy.syntax;

import static org.hamcrest.MatcherAssert.assertThat;
import static org.hamcrest.Matchers.equalTo;
import static org.junit.jupiter.api.Assertions.assertEquals;

import java.io.IOException;
import java.nio.file.Files;
Expand Down Expand Up @@ -54,7 +53,7 @@ public void testRunner(Path filename) {
String formatted = Formatter.format(tree, 120);
String expected = IoUtils.readUtf8File(formattedFile);

assertThat(formatted, equalTo(expected));
assertEquals(expected, formatted);
}

public static List<Path> tests() throws Exception {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,19 +23,24 @@ use smithy.api#String

/// 12 (keep)
@deprecated
/// 13 (change)
// 13 (change)
structure Foo {
/// 14 (keep)
@length(
// 15 (change)
min: 1
)
/// 16 (change)
// 16 (change)
@since("1.x")
/// 17 (TODO: change)
// 17 (change)
bar: String

// 18 (change)
/// 17a (keep)
@length(min: 1)
baz: String = ""

/// 18 (TODO: Fix trailing comment after VALUE_ASSIGNMENT)

}

// 19 (change)
Expand All @@ -45,4 +50,10 @@ apply Foo @tags(["a"])
list Baz {
member: Integer
}
// 21 (change)

structure Foo2 {
foo2: String

// 21 (change)
}
// 22 (change)
Original file line number Diff line number Diff line change
Expand Up @@ -32,9 +32,14 @@ structure Foo {
)
/// 16 (change)
@since("1.x")
/// 17 (TODO: change)
/// 17 (change)
bar: String
/// 18 (change)

/// 17a (keep)
@length(min: 1)
baz: String = ""

/// 18 (TODO: Fix trailing comment after VALUE_ASSIGNMENT)
}

/// 19 (change)
Expand All @@ -45,4 +50,10 @@ list Baz {
member: Integer
}

/// 21 (change)
structure Foo2 {
foo2: String

/// 21 (change)
}

/// 22 (change)

0 comments on commit e71e5c8

Please sign in to comment.