Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add backticks to diff messages for trait changes #2075

Merged
merged 4 commits into from
Jan 24, 2024
Merged

Add backticks to diff messages for trait changes #2075

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
5351512
Add backticks to diff messages for trait changes
rchache Dec 15, 2023
dc54402
Clean up / refactor
rchache Dec 25, 2023
2297e71
minor string changes
rchache Jan 16, 2024
c20cbf7
Standardized line separators.
rchache Jan 17, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
50 changes: 25 additions & 25 deletions smithy-cli/src/it/java/software/amazon/smithy/cli/FormatCommandTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,14 +32,14 @@ public void formatsSingleFile() {
IntegUtils.run("bad-formatting", ListUtils.of("format", "model/other.smithy"), result -> {
assertThat(result.getExitCode(), equalTo(0));

String model = result.getFile("model/other.smithy").replace("\r\n", "\n");
assertThat(model, equalTo("$version: \"2.0\"\n"
+ "\n"
+ "namespace smithy.example\n"
+ "\n"
+ "string MyString\n"
+ "\n"
+ "string MyString2\n"));
String model = result.getFile("model/other.smithy");
assertThat(model, equalTo(String.format("$version: \"2.0\"%n"
+ "%n"
+ "namespace smithy.example%n"
+ "%n"
+ "string MyString%n"
+ "%n"
+ "string MyString2%n")));
});
}

Expand All @@ -48,24 +48,24 @@ public void formatsDirectory() {
IntegUtils.run("bad-formatting", ListUtils.of("format", "model"), result -> {
assertThat(result.getExitCode(), equalTo(0));

String main = result.getFile("model/main.smithy").replace("\r\n", "\n");
assertThat(main, equalTo("$version: \"2.0\"\n"
+ "\n"
+ "metadata this_is_a_long_string = {\n"
+ " this_is_a_long_string1: \"a\"\n"
+ " this_is_a_long_string2: \"b\"\n"
+ " this_is_a_long_string3: \"c\"\n"
+ " this_is_a_long_string4: \"d\"\n"
+ "}\n"));
String main = result.getFile("model/main.smithy");
assertThat(main, equalTo(String.format("$version: \"2.0\"%n"
+ "%n"
+ "metadata this_is_a_long_string = {%n"
+ " this_is_a_long_string1: \"a\"%n"
+ " this_is_a_long_string2: \"b\"%n"
+ " this_is_a_long_string3: \"c\"%n"
+ " this_is_a_long_string4: \"d\"%n"
+ "}%n")));

String other = result.getFile("model/other.smithy").replace("\r\n", "\n");
assertThat(other, equalTo("$version: \"2.0\"\n"
+ "\n"
+ "namespace smithy.example\n"
+ "\n"
+ "string MyString\n"
+ "\n"
+ "string MyString2\n"));
String other = result.getFile("model/other.smithy");
assertThat(other, equalTo(String.format("$version: \"2.0\"%n"
+ "%n"
+ "namespace smithy.example%n"
+ "%n"
+ "string MyString%n"
+ "%n"
+ "string MyString2%n")));
});
}
}
9 changes: 5 additions & 4 deletions smithy-diff/src/main/java/software/amazon/smithy/diff/evaluators/ModifiedTrait.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@
import software.amazon.smithy.model.traits.synthetic.SyntheticEnumTrait;
import software.amazon.smithy.model.validation.Severity;
import software.amazon.smithy.model.validation.ValidationEvent;
import software.amazon.smithy.model.validation.ValidationUtils;
import software.amazon.smithy.utils.ListUtils;
import software.amazon.smithy.utils.SetUtils;
import software.amazon.smithy.utils.StringUtils;
Expand Down Expand Up @@ -227,7 +228,7 @@ List<ValidationEvent> validate(
}

String message;
String pretty = Node.prettyPrintJson(right.toNode());
String pretty = ValidationUtils.tickedPrettyPrintedNode(right);
if (path.isEmpty()) {
message = String.format("Added trait `%s` with value %s", trait, pretty);
} else {
Expand Down Expand Up @@ -260,7 +261,7 @@ List<ValidationEvent> validate(
return Collections.emptyList();
}

String pretty = Node.prettyPrintJson(left.toNode());
String pretty = ValidationUtils.tickedPrettyPrintedNode(left);
String message;
if (path.isEmpty()) {
message = String.format("Removed trait `%s`. Previous trait value: %s", trait, pretty);
Expand Down Expand Up @@ -294,8 +295,8 @@ List<ValidationEvent> validate(
return Collections.emptyList();
}

String leftPretty = Node.prettyPrintJson(left.toNode());
String rightPretty = Node.prettyPrintJson(right.toNode());
String leftPretty = ValidationUtils.tickedPrettyPrintedNode(left);
String rightPretty = ValidationUtils.tickedPrettyPrintedNode(right);
String message;
if (path.isEmpty()) {
message = String.format("Changed trait `%s` from %s to %s", trait, leftPretty, rightPretty);
Expand Down
9 changes: 5 additions & 4 deletions smithy-diff/src/main/java/software/amazon/smithy/diff/evaluators/TraitBreakingChange.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@
import software.amazon.smithy.model.traits.Trait;
import software.amazon.smithy.model.traits.TraitDefinition;
import software.amazon.smithy.model.validation.ValidationEvent;
import software.amazon.smithy.model.validation.ValidationUtils;
import software.amazon.smithy.utils.StringUtils;

/**
Expand Down Expand Up @@ -214,15 +215,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 = ValidationUtils.tickedPrettyPrintedNode(left);
String rightPretty = ValidationUtils.tickedPrettyPrintedNode(right);

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)) {
return String.format("Added trait `%s`", trait.getId());
} else {
return String.format("Added trait `%s` with value %s", trait.getId(), rightPretty);
Expand All @@ -231,7 +232,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)) {
return String.format("Removed trait `%s`", trait.getId());
} else {
return String.format("Removed trait `%s`. Previous trait value: %s",
Expand Down
14 changes: 7 additions & 7 deletions ...-diff/src/test/java/software/amazon/smithy/diff/evaluators/ChangedPaginatedTraitTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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
Expand All @@ -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
Expand All @@ -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
Expand Down Expand Up @@ -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
Expand All @@ -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
Expand All @@ -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`"));
}
}
86 changes: 54 additions & 32 deletions smithy-diff/src/test/java/software/amazon/smithy/diff/evaluators/ModifiedTraitTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -160,9 +160,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`"
));
}

Expand All @@ -186,8 +186,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`"
));
}

Expand All @@ -210,11 +210,17 @@ 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`",
String.format("Removed trait contents from `smithy.example#aTrait` at path `/foo`. Removed value: %n"
+ "```%n"
+ "[%n"
+ " \"1\",%n"
+ " \"2\",%n"
+ " \"3\"%n"
+ "]%n"
+ "```%n")
));
}

Expand All @@ -237,10 +243,18 @@ 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`",
String.format("Removed trait contents from `smithy.example#aTrait` at path `/foo`. "
+ "Removed value: %n"
+ "```%n"
+ "[%n"
+ " \"1\",%n"
+ " \"2\",%n"
+ " \"3\"%n"
+ "]%n"
+ "```%n")
));
}

Expand All @@ -263,10 +277,18 @@ 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`",
String.format("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"
+ "```%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`"
));
}

Expand All @@ -287,8 +309,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`"
));
}

Expand All @@ -311,18 +333,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`"
));
}

Expand Down
Loading