-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Fix an inconsistency in MapObjectEncoder #657
Conversation
Codecov Report
@@ Coverage Diff @@
## master #657 +/- ##
==========================================
+ Coverage 97.38% 97.43% +0.04%
==========================================
Files 40 40
Lines 2102 2102
==========================================
+ Hits 2047 2048 +1
+ Misses 47 46 -1
Partials 8 8
Continue to review full report at Codecov.
|
2a60474
to
293c067
Compare
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.
Thank you for opening this PR! It's been a while since we wrote this code, so I'm honestly not sure why there's no AppendByteString
on the PrimitiveArrayEncoder
interface. It was probably an oversight - good on you for catching it!
Unfortunately, we can't add this method today, since it breaks backward compatibility. (Adding a method to an interface immediately breaks all third-party implementations of that interface.) I don't want to forget about this, though, so can you please add an item to #388? If we ever accumulate enough changes that we're ready to cut a v2, we'll fix this oversight too.
That said, I would love to pull in your fix to the in-memory encoder. Can you drop the second commit on this PR and fix up the description?
Also, thank you for pitching in to help answer issues. It's immensely helpful. |
293c067
to
2ed8557
Compare
Sorry for the long delay, I almost forgot about this! I've removed the ArrayEncoder changes, and put them in a separate PR. Since you have various "v2" issues queued up, could you make a branch so that you can merge the PRs - and there's no rush to actually finish and release the v2 API. |
Ping - I made the changes requested, this PR now only contains the changes that are OK to merge to master. |
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.
Looks good to me, thanks for the fix.
* master: README: Switch to travis-ci.com for badge (uber-go#709) Fix changelog links for 675 Prep for 1.10.0 release, update CHANGELOG (uber-go#705) Add Go 1.12 for Travis (uber-go#707) Fix call depth of standard logger in go1.12 (uber-go#706) Fix inconsistency between MapObjectEncoder's AddByteString and AppendByteString (uber-go#657) Disable HTMLEscape in reflect JSON encoder (uber-go#704)
For some reason, ObjectEncoder has
AddBinary
andAddByteString
methods, but ArrayEncoder only hasAppendByteString
. This PR adds the missing (forgotten?)AppendBinary
method.Also, I've fixed up a small inconsistency in the MapObjectEncoder that's used in the tests.
Two independent commits - can be reviewed/merged separately (or split into two PRs if you prefer).