-
Notifications
You must be signed in to change notification settings - Fork 104
[682] Correct the fix for 670 and return the type if the type has alr… #683
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.
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
base: master
Are you sure you want to change the base?
Conversation
…eady been resolved. Signed-off-by: James R. Perkins <[email protected]>
KyleAure
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This fix looks good.
I think all we are missing from the tests are appropriate de-serialization testing for the same scenario. This looks like common practice in this test class so I added those suggestions below.
| // Use a new instance of Jsonb to avoid any caching | ||
| try (var jsonb = JsonbBuilder.create()) { | ||
| assertEquals(expectedJson, jsonb.toJson(container)); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| // Use a new instance of Jsonb to avoid any caching | |
| try (var jsonb = JsonbBuilder.create()) { | |
| assertEquals(expectedJson, jsonb.toJson(container)); | |
| } | |
| // Use a new instance of Jsonb to avoid any caching | |
| try (var jsonb = JsonbBuilder.create()) { | |
| assertEquals(expectedJson, jsonb.toJson(container)); | |
| TreeContainer<TreeElement> result = jsonb.fromJson(expectedJson, new TestTypeToken<TreeContainer<TreeElement>>() {}.getType()); | |
| assertIterableEquals(container.getTree().getChildren(), result.getTree().getChildren()); | |
| } |
Requires addition of:
import static org.junit.jupiter.api.Assertions.assertIterableEquals;| // Use a new instance of Jsonb to avoid any caching | ||
| try (var jsonb = JsonbBuilder.create()) { | ||
| assertEquals(expectedJson, jsonb.toJson(container)); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| // Use a new instance of Jsonb to avoid any caching | |
| try (var jsonb = JsonbBuilder.create()) { | |
| assertEquals(expectedJson, jsonb.toJson(container)); | |
| } | |
| // Use a new instance of Jsonb to avoid any caching | |
| try (var jsonb = JsonbBuilder.create()) { | |
| assertEquals(expectedJson, jsonb.toJson(container)); | |
| ListContainer<TreeElement> result = jsonb.fromJson(expectedJson, new TestTypeToken<ListContainer<TreeElement>>() {}.getType()); | |
| assertIterableEquals(container.getList(), result.getList()); | |
| } |
Requires addition of:
import static org.junit.jupiter.api.Assertions.assertIterableEquals;| public class TreeElement implements TreeTypeContainer<TreeElement> { | ||
| public class TreeElement extends TreeTypeContainer<TreeElement> { | ||
|
|
||
| private final String name; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| private final String name; | |
| private String name; | |
| public TreeElement() { | |
| } |
suggestion: allow for deserialization
| public void setChildren(final List<TreeElement> children) { | ||
| this.children = children; | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| } | |
| public void setName(final String name) { | |
| this.name = name; | |
| } | |
| @Override | |
| public boolean equals(Object o) { | |
| if (this == o) { | |
| return true; | |
| } | |
| if (o == null || getClass() != o.getClass()) { | |
| return false; | |
| } | |
| if (!super.equals(o)) { | |
| return false; | |
| } | |
| TreeElement that = (TreeElement) o; | |
| return name != null ? name.equals(that.name) : that.name == null; | |
| } | |
| } |
suggestion: allow for deserialization and assertions.
| public void setChildren(final List<T> children) { | ||
| this.children = children; | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| } | |
| @Override | |
| public boolean equals(Object o) { | |
| if (this == o) { | |
| return true; | |
| } | |
| if (o == null || getClass() != o.getClass()) { | |
| return false; | |
| } | |
| TreeTypeContainer<?> that = (TreeTypeContainer<?>) o; | |
| if (children == null) { | |
| return that.children == null; | |
| } | |
| return children.containsAll(that.children) && that.children.containsAll(children); | |
| } | |
| } |
Suggestion: allow for assertions.
…eady been resolved.
resolves #682