Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
ZMQ PUB/SUB JSON Types #330
ZMQ PUB/SUB JSON Types #330
Changes from 13 commits
03e1939
57fbb6b
e5875ed
89b964d
40a26bc
9817bbd
811c921
7292bba
d4a6998
f76c84d
d301deb
d120285
d06d4b8
bf48d4d
0086962
863a77d
ce822f9
ef74d21
dbaad27
2f3d176
a719f7a
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
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.
IMO I would rather have worse error messages on tests than pull an extra dep in
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.
I pulled it in as a dev-only dependency, because testing JSON equality is a very non-trivial thing to do. Client readers are not supposed to care about whitespace or the order of fields within an object. If the input object is a map, its field output emit order can even vary between test runs.
In Golang, the feature to test JSON equality is built right into the most popular testing library (
testify
).assert-json-diff
appeared to be the most widely used option for testing JSON-backwards-compatibility in Rust.I did verify that the library differentiates between empty lists and nil lists, and the other types of backwards compatibility that we care about.
If, after reading this, we still don't want to use the
assert-json-diff
crate, let me know and I'll find a way to make the tests work without it. We are not using any maps, so it should be possible. The expected values will probably have to be on single, super-long lines, so they won't be human readable. Or I can get the test samples formatted in the same way as serde's pretty print and use multi-line raw strings.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.
Seen as it is only a dev dep we can keep it for now, although it might get removed to reduce the number of deps in the future