-
Notifications
You must be signed in to change notification settings - Fork 100
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
Json Protocol support #156
Json Protocol support #156
Conversation
Codecov Report
@@ Coverage Diff @@
## master #156 +/- ##
============================================
+ Coverage 65.51% 66.13% +0.61%
- Complexity 938 1012 +74
============================================
Files 89 90 +1
Lines 4843 5268 +425
Branches 562 611 +49
============================================
+ Hits 3173 3484 +311
- Misses 1431 1524 +93
- Partials 239 260 +21
Continue to review full report at Codecov.
|
@benjamin-bader did you have time to look at this code? |
|
||
@Test | ||
public void binary() throws Exception { | ||
protocol.writeBinary(ByteString.encodeUtf8("foobar")); |
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 think we should exercise the base-64 coder and decoder more thoroughly here. Let's generate an array of bytes from 0..255 and encode that, so that we hit all of the encoding/decoding table entries.
I'm assuming that those are copied verbatim from Apache, and so are almost certainly correct, but we shouldn't assume such things in tests :)
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.
Yes, your assumption is correct. And yeah, I'll add tests for the decoding.
Yes, sorry for the delay - I've been an overloaded maintainer on quite a few projects for the past while. This change is nice and self-contained, which is great, but is also large. Am I correct in thinking that this is mainly a copy-and-adapt implementation based on Apache's? |
@benjamin-bader I understand - don't worry about it, it's not urgent. You're right, Apache's implementation was the basis for this PR. |
I wrote a test for the base64 decoding (see e33a66d). But then I realized we can use the base64 decoding from the Okio @benjamin-bader let me know if you want anything else changed. |
@benjamin-bader is there anything I can do to help you review and merge this work? |
Thanks for contributing! |
Add Json Protocol support
as it was added in 4c5e96b / microsoft#156
I started from the official Thrift implementation, made it work and tried to clean up the style in the process. Let me know if there's anything you'd like me to change, happy to do it.
I already signed the contributor agreement for an earlier PR.
Closes #154.