Add backticks to diff messages for trait changes

rchache · Dec 15, 2023 · 5351512 · 5351512
1 parent 5753eb2
commit 5351512
Show file tree

Hide file tree

Showing 6 changed files with 84 additions and 52 deletions.
diff --git a/smithy-diff/src/main/java/software/amazon/smithy/diff/evaluators/ModifiedTrait.java b/smithy-diff/src/main/java/software/amazon/smithy/diff/evaluators/ModifiedTrait.java
@@ -227,7 +227,7 @@ List<ValidationEvent> validate(
                 }
 
                 String message;
-                String pretty = Node.prettyPrintJson(right.toNode());
+                String pretty = Node.prettyPrintJsonWithBackticks(right.toNode());
                 if (path.isEmpty()) {
                     message = String.format("Added trait `%s` with value %s", trait, pretty);
                 } else {
@@ -260,7 +260,7 @@ List<ValidationEvent> validate(
                     return Collections.emptyList();
                 }
 
-                String pretty = Node.prettyPrintJson(left.toNode());
+                String pretty = Node.prettyPrintJsonWithBackticks(left.toNode());
                 String message;
                 if (path.isEmpty()) {
                     message = String.format("Removed trait `%s`. Previous trait value: %s", trait, pretty);
@@ -294,8 +294,8 @@ List<ValidationEvent> validate(
                     return Collections.emptyList();
                 }
 
-                String leftPretty = Node.prettyPrintJson(left.toNode());
-                String rightPretty = Node.prettyPrintJson(right.toNode());
+                String leftPretty = Node.prettyPrintJsonWithBackticks(left.toNode());
+                String rightPretty = Node.prettyPrintJsonWithBackticks(right.toNode());
                 String message;
                 if (path.isEmpty()) {
                     message = String.format("Changed trait `%s` from %s to %s", trait, leftPretty, rightPretty);

diff --git a/smithy-diff/src/main/java/software/amazon/smithy/diff/evaluators/TraitBreakingChange.java b/smithy-diff/src/main/java/software/amazon/smithy/diff/evaluators/TraitBreakingChange.java
@@ -214,15 +214,15 @@ private TraitDefinition.ChangeType isChangeBreaking(TraitDefinition.ChangeType t
         }
 
         private String createBreakingMessage(TraitDefinition.ChangeType type, String path, Node left, Node right) {
-            String leftPretty = Node.prettyPrintJson(left.toNode());
-            String rightPretty = Node.prettyPrintJson(right.toNode());
+            String leftPretty = Node.prettyPrintJsonWithBackticks(left.toNode());
+            String rightPretty = Node.prettyPrintJsonWithBackticks(right.toNode());
 
             switch (type) {
                 case ADD:
                     if (!path.isEmpty()) {
                         return String.format("Added trait contents to `%s` at path `%s` with value %s",
                                              trait.getId(), path, rightPretty);
-                    } else if (rightPretty.equals("{}")) {
+                    } else if (Node.objectNode().equals(right.toNode())) {
                         return String.format("Added trait `%s`", trait.getId());
                     } else {
                         return String.format("Added trait `%s` with value %s", trait.getId(), rightPretty);
@@ -231,7 +231,7 @@ private String createBreakingMessage(TraitDefinition.ChangeType type, String pat
                     if (!path.isEmpty()) {
                         return String.format("Removed trait contents from `%s` at path `%s`. Removed value: %s",
                                              trait.getId(), path, leftPretty);
-                    } else if (leftPretty.equals("{}")) {
+                    } else if (Node.objectNode().equals(left.toNode())) {
                         return String.format("Removed trait `%s`", trait.getId());
                     } else {
                         return String.format("Removed trait `%s`. Previous trait value: %s",

diff --git a/...-diff/src/test/java/software/amazon/smithy/diff/evaluators/ChangedPaginatedTraitTest.java b/...-diff/src/test/java/software/amazon/smithy/diff/evaluators/ChangedPaginatedTraitTest.java
@@ -74,7 +74,7 @@ public void detectsRemovalOfItems() {
 
         assertThat(changedTraitEvents.size(), equalTo(1));
         assertThat(changedTraitEvents.get(0).getMessage(),
-                   containsString("Removed trait contents from `smithy.api#paginated` at path `/items`. Removed value: \"things\""));
+                   containsString("Removed trait contents from `smithy.api#paginated` at path `/items`. Removed value: `things`"));
     }
 
     @Test
@@ -91,7 +91,7 @@ public void detectsAdditionOfItems() {
 
         assertThat(changedTraitEvents.size(), equalTo(1));
         assertThat(changedTraitEvents.get(0).getMessage(),
-                   containsString("Added trait contents to `smithy.api#paginated` at path `/items` with value \"things\""));
+                   containsString("Added trait contents to `smithy.api#paginated` at path `/items` with value `things`"));
     }
 
     @Test
@@ -108,7 +108,7 @@ public void detectsChangeToItems() {
 
         assertThat(changedTraitEvents.size(), equalTo(1));
         assertThat(changedTraitEvents.get(0).getMessage(),
-                   containsString("Changed trait contents of `smithy.api#paginated` at path `/items` from \"things\" to \"otherThings\""));
+                   containsString("Changed trait contents of `smithy.api#paginated` at path `/items` from `things` to `otherThings`"));
     }
 
     @Test
@@ -125,7 +125,7 @@ public void detectsRemovalOfPageSize() {
 
         assertThat(changedTraitEvents.size(), equalTo(1));
         assertThat(changedTraitEvents.get(0).getMessage(),
-                   containsString("Removed trait contents from `smithy.api#paginated` at path `/pageSize`. Removed value: \"maxResults\""));
+                   containsString("Removed trait contents from `smithy.api#paginated` at path `/pageSize`. Removed value: `maxResults`"));
     }
 
     @Test
@@ -157,7 +157,7 @@ public void detectsChangeToPageSize() {
 
         assertThat(changedTraitEvents.size(), equalTo(1));
         assertThat(changedTraitEvents.get(0).getMessage(),
-                   containsString("Changed trait contents of `smithy.api#paginated` at path `/pageSize` from \"maxResults\" to \"otherMaxResults\""));
+                   containsString("Changed trait contents of `smithy.api#paginated` at path `/pageSize` from `maxResults` to `otherMaxResults`"));
     }
 
     @Test
@@ -173,7 +173,7 @@ public void detectsAnyChangeToInputToken() {
 
         assertThat(changedTraitEvents.size(), equalTo(1));
         assertThat(changedTraitEvents.get(0).getMessage(),
-                   containsString("Changed trait contents of `smithy.api#paginated` at path `/inputToken` from \"token\" to \"otherToken\""));
+                   containsString("Changed trait contents of `smithy.api#paginated` at path `/inputToken` from `token` to `otherToken`"));
     }
 
     @Test
@@ -189,6 +189,6 @@ public void detectsAnyChangeToOutputToken() {
 
         assertThat(changedTraitEvents.size(), equalTo(1));
         assertThat(changedTraitEvents.get(0).getMessage(),
-                   containsString("Changed trait contents of `smithy.api#paginated` at path `/outputToken` from \"token\" to \"otherToken\""));
+                   containsString("Changed trait contents of `smithy.api#paginated` at path `/outputToken` from `token` to `otherToken`"));
     }
 }
diff --git a/smithy-diff/src/test/java/software/amazon/smithy/diff/evaluators/ModifiedTraitTest.java b/smithy-diff/src/test/java/software/amazon/smithy/diff/evaluators/ModifiedTraitTest.java
@@ -25,6 +25,7 @@
 import java.util.Arrays;
 import java.util.Collection;
 import java.util.List;
+import java.util.logging.Logger;
 import java.util.stream.Collectors;
 import org.junit.jupiter.api.Test;
 import org.junit.jupiter.params.ParameterizedTest;
@@ -160,9 +161,9 @@ public void modifiedShapeNoTag() {
                 equalTo(2L));
 
         assertThat(messages, containsInAnyOrder(
-                "Changed trait `smithy.example#b` from \"hello\" to \"hello!\"",
-                "Removed trait `smithy.example#a`. Previous trait value: {}",
-                "Added trait `smithy.example#c` with value \"foo\""
+                "Changed trait `smithy.example#b` from `hello` to `hello!`",
+                "Removed trait `smithy.example#a`. Previous trait value: `{}`",
+                "Added trait `smithy.example#c` with value `foo`"
         ));
     }
 
@@ -186,8 +187,8 @@ public void findsDifferencesInTraitValues() {
                 equalTo(2L));
 
         assertThat(messages, containsInAnyOrder(
-                "Added trait contents to `smithy.example#aTrait` at path `/bar` with value \"no\"",
-                "Changed trait contents of `smithy.example#aTrait` at path `/baz/foo` from \"bye\" to \"adios\""
+                "Added trait contents to `smithy.example#aTrait` at path `/bar` with value `no`",
+                "Changed trait contents of `smithy.example#aTrait` at path `/baz/foo` from `bye` to `adios`"
         ));
     }
 
@@ -210,11 +211,13 @@ public void findsDifferencesInListTraitValues() {
         assertThat(events.stream().filter(e -> !e.getMessage().contains("Removed"))
                 .filter(e -> e.getSourceLocation().getFilename().endsWith("b.smithy")).count(), equalTo(2L));
         assertThat(messages, containsInAnyOrder(
-                "Changed trait contents of `smithy.example#aTrait` at path `/foo/1` from \"b\" to \"B\"",
-                "Added trait contents to `smithy.example#aTrait` at path `/foo/3` with value \"4\"",
-                "Removed trait contents from `smithy.example#aTrait` at path `/foo/2`. Removed value: \"3\"",
-                "Removed trait contents from `smithy.example#aTrait` at path `/foo`. Removed value: "
-                + "[\n    \"1\",\n    \"2\",\n    \"3\"\n]"
+                "Changed trait contents of `smithy.example#aTrait` at path `/foo/1` from `b` to `B`",
+                "Added trait contents to `smithy.example#aTrait` at path `/foo/3` with value `4`",
+                "Removed trait contents from `smithy.example#aTrait` at path `/foo/2`. Removed value: `3`",
+                "Removed trait contents from `smithy.example#aTrait` at path `/foo`. Removed value: \n"
+                        + "```\n"
+                        + "[\n    \"1\",\n    \"2\",\n    \"3\"\n]\n"
+                        + "```"
         ));
     }
 
@@ -237,10 +240,14 @@ public void findsDifferencesInSetTraitValues() {
         assertThat(events.stream().filter(e -> !e.getMessage().contains("Removed"))
                 .filter(e -> e.getSourceLocation().getFilename().endsWith("b.smithy")).count(), equalTo(2L));
         assertThat(messages, containsInAnyOrder(
-                "Changed trait contents of `smithy.example#aTrait` at path `/foo/1` from \"b\" to \"B\"",
-                "Added trait contents to `smithy.example#aTrait` at path `/foo/3` with value \"4\"",
-                "Removed trait contents from `smithy.example#aTrait` at path `/foo/2`. Removed value: \"3\"",
-                "Removed trait contents from `smithy.example#aTrait` at path `/foo`. Removed value: [\n    \"1\",\n    \"2\",\n    \"3\"\n]"
+                "Changed trait contents of `smithy.example#aTrait` at path `/foo/1` from `b` to `B`",
+                "Added trait contents to `smithy.example#aTrait` at path `/foo/3` with value `4`",
+                "Removed trait contents from `smithy.example#aTrait` at path `/foo/2`. Removed value: `3`",
+                "Removed trait contents from `smithy.example#aTrait` at path `/foo`. "
+                        + "Removed value: \n"
+                        + "```\n"
+                        + "[\n    \"1\",\n    \"2\",\n    \"3\"\n]\n"
+                        + "```"
         ));
     }
 
@@ -263,10 +270,14 @@ public void findsDifferencesInMapTraitValues() {
         assertThat(events.stream().filter(e -> !e.getMessage().contains("Removed"))
                 .filter(e -> e.getSourceLocation().getFilename().endsWith("b.smithy")).count(), equalTo(2L));
         assertThat(messages, containsInAnyOrder(
-                "Changed trait contents of `smithy.example#aTrait` at path `/foo/bam` from \"b\" to \"B\"",
-                "Removed trait contents from `smithy.example#aTrait` at path `/foo`. Removed value: {\n    \"baz\": \"1\",\n    \"bam\": \"2\",\n    \"boo\": \"3\"\n}",
-                "Added trait contents to `smithy.example#aTrait` at path `/foo/qux` with value \"4\"",
-                "Removed trait contents from `smithy.example#aTrait` at path `/foo/boo`. Removed value: \"3\""
+                "Changed trait contents of `smithy.example#aTrait` at path `/foo/bam` from `b` to `B`",
+                "Removed trait contents from `smithy.example#aTrait` at path `/foo`. "
+                        + "Removed value: \n"
+                        + "```\n"
+                        + "{\n    \"baz\": \"1\",\n    \"bam\": \"2\",\n    \"boo\": \"3\"\n}\n"
+                        + "```",
+                "Added trait contents to `smithy.example#aTrait` at path `/foo/qux` with value `4`",
+                "Removed trait contents from `smithy.example#aTrait` at path `/foo/boo`. Removed value: `3`"
         ));
     }
 
@@ -287,8 +298,8 @@ public void findsDifferencesInUnionTraitValues() {
         assertThat(events.stream().filter(e -> e.getSourceLocation().getFilename().endsWith("b.smithy")).count(),
                 equalTo(2L));
         assertThat(messages, containsInAnyOrder(
-                "Changed trait contents of `smithy.example#aTrait` at path `/baz/foo` from \"a\" to \"b\"",
-                "Changed trait contents of `smithy.example#aTrait` at path `/baz/baz` from \"a\" to \"b\""
+                "Changed trait contents of `smithy.example#aTrait` at path `/baz/foo` from `a` to `b`",
+                "Changed trait contents of `smithy.example#aTrait` at path `/baz/baz` from `a` to `b`"
         ));
     }
 
@@ -311,18 +322,18 @@ public void findsDifferencesInTraitValuesOfAllSeverities() {
         assertThat(events.stream().filter(e -> e.getMessage().contains("Changed") || e.getMessage().contains("Added"))
                 .filter(e -> e.getSourceLocation().getFilename().endsWith("b.smithy")).count(), equalTo(9L));
         assertThat(messages, containsInAnyOrder(
-                "Added trait contents to `smithy.example#aTrait` at path `/a` with value \"a\"",
-                "Removed trait contents from `smithy.example#aTrait` at path `/b`. Removed value: \"a\"",
-                "Changed trait contents of `smithy.example#aTrait` at path `/c` from \"a\" to \"c\"",
-                "Changed trait contents of `smithy.example#aTrait` at path `/d` from \"a\" to \"d\"",
-                "Added trait contents to `smithy.example#aTrait` at path `/e` with value \"a\"",
-                "Removed trait contents from `smithy.example#aTrait` at path `/f`. Removed value: \"a\"",
-                "Changed trait contents of `smithy.example#aTrait` at path `/g` from \"a\" to \"h\"",
-                "Changed trait contents of `smithy.example#aTrait` at path `/h` from \"a\" to \"h\"",
-                "Added trait contents to `smithy.example#aTrait` at path `/i` with value \"a\"",
-                "Removed trait contents from `smithy.example#aTrait` at path `/j`. Removed value: \"a\"",
-                "Changed trait contents of `smithy.example#aTrait` at path `/k` from \"a\" to \"k\"",
-                "Changed trait contents of `smithy.example#aTrait` at path `/l` from \"a\" to \"l\""
+                "Added trait contents to `smithy.example#aTrait` at path `/a` with value `a`",
+                "Removed trait contents from `smithy.example#aTrait` at path `/b`. Removed value: `a`",
+                "Changed trait contents of `smithy.example#aTrait` at path `/c` from `a` to `c`",
+                "Changed trait contents of `smithy.example#aTrait` at path `/d` from `a` to `d`",
+                "Added trait contents to `smithy.example#aTrait` at path `/e` with value `a`",
+                "Removed trait contents from `smithy.example#aTrait` at path `/f`. Removed value: `a`",
+                "Changed trait contents of `smithy.example#aTrait` at path `/g` from `a` to `h`",
+                "Changed trait contents of `smithy.example#aTrait` at path `/h` from `a` to `h`",
+                "Added trait contents to `smithy.example#aTrait` at path `/i` with value `a`",
+                "Removed trait contents from `smithy.example#aTrait` at path `/j`. Removed value: `a`",
+                "Changed trait contents of `smithy.example#aTrait` at path `/k` from `a` to `k`",
+                "Changed trait contents of `smithy.example#aTrait` at path `/l` from `a` to `l`"
         ));
     }
 

diff --git a/...hy-diff/src/test/java/software/amazon/smithy/diff/evaluators/TraitBreakingChangeTest.java b/...hy-diff/src/test/java/software/amazon/smithy/diff/evaluators/TraitBreakingChangeTest.java
@@ -131,7 +131,7 @@ public void canPathIntoListMembers() {
                 assertThat(events.get(0).getId(), equalTo("TraitBreakingChange.Update.smithy.example#exampleTrait"));
                 assertThat(events.get(0).getMessage(),
                            equalTo("Changed trait contents of `smithy.example#exampleTrait` at path `/1` "
-                                   + "from \"b\" to \"B\""));
+                                   + "from `b` to `B`"));
             }
         );
     }
@@ -147,7 +147,7 @@ public void canPathIntoMapKeys() {
                     assertThat(events.get(0).getId(), equalTo("TraitBreakingChange.Remove.smithy.example#exampleTrait"));
                     assertThat(events.get(0).getMessage(),
                                equalTo("Removed trait contents from `smithy.example#exampleTrait` at path `/b`. "
-                                       + "Removed value: \"B\""));
+                                       + "Removed value: `B`"));
                 }
         );
     }
@@ -166,7 +166,7 @@ public void canPathIntoMapValues() {
                 assertThat(events.get(0).getId(), equalTo("TraitBreakingChange.Update.smithy.example#exampleTrait"));
                 assertThat(events.get(0).getMessage(),
                            equalTo("Changed trait contents of `smithy.example#exampleTrait` at path `/b` "
-                                   + "from \"B\" to \"_B_\""));
+                                   + "from `B` to `_B_`"));
             }
         );
     }
@@ -184,7 +184,7 @@ public void canPathIntoStructureMembers() {
                 assertThat(events.get(0).getId(), equalTo("TraitBreakingChange.Remove.smithy.example#exampleTrait"));
                 assertThat(events.get(0).getMessage(),
                            equalTo("Removed trait contents from `smithy.example#exampleTrait` at path "
-                                   + "`/foo/bar`. Removed value: \"hi\""));
+                                   + "`/foo/bar`. Removed value: `hi`"));
             }
         );
     }
@@ -202,7 +202,7 @@ public void canPathIntoUnionMembers() {
                 assertThat(events.get(0).getId(), equalTo("TraitBreakingChange.Update.smithy.example#exampleTrait"));
                 assertThat(events.get(0).getMessage(),
                            equalTo("Changed trait contents of `smithy.example#exampleTrait` at path "
-                                   + "`/foo` from \"hi\" to \"bye\""));
+                                   + "`/foo` from `hi` to `bye`"));
             }
         );
     }

diff --git a/smithy-model/src/main/java/software/amazon/smithy/model/node/Node.java b/smithy-model/src/main/java/software/amazon/smithy/model/node/Node.java
@@ -155,6 +155,27 @@ public static String prettyPrintJson(Node node, String indentString) {
         return NodeHandler.prettyPrint(node, indentString);
     }
 
+    /**
+     * Writes the contents of a Node to a pretty-printed JSON string,
+     * and surrounds the result in backticks.
+     *
+     * @param node Node to write.
+     * @return Returns a string suitable for Markdown rendering.
+     */
+    public static String prettyPrintJsonWithBackticks(Node node) {
+        String prettyPrinted = prettyPrintJson(node);
+        if (prettyPrinted.contains(System.lineSeparator()) || prettyPrinted.contains("\n")) {
+            return System.lineSeparator() + "```" + System.lineSeparator()
+                    + prettyPrinted + System.lineSeparator()
+                    + "```";
+        } else if (prettyPrinted.startsWith("\"") && prettyPrinted.endsWith("\"")) {
+            // for pure strings, replace the quotes with backticks
+            return "`" + prettyPrinted.substring(1, prettyPrinted.length() - 1) + "`";
+        } else {
+            return "`" + prettyPrinted + "`";
+        }
+    }
+
     /**
      * Writes the contents of a Node to a non-pretty-printed JSON string.
      *