Allow representations of nillable primitive field values#758
Allow representations of nillable primitive field values#758abhinav merged 5 commits intouber-go:masterfrom
Conversation
|
|
Codecov Report
@@ Coverage Diff @@
## master #758 +/- ##
=========================================
+ Coverage 98.09% 98.19% +0.1%
=========================================
Files 42 42
Lines 2150 2269 +119
=========================================
+ Hits 2109 2228 +119
Misses 33 33
Partials 8 8
Continue to review full report at Codecov.
|
| // AppendBytes appends a byte slice to the Buffer. | ||
| func (b *Buffer) AppendBytes(bs []byte) { | ||
| b.bs = append(b.bs, bs...) | ||
| } |
There was a problem hiding this comment.
Why create this method instead of just using Write?
Since this package and type is public, we'd prefer to not introduce new
methods if possible.
There was a problem hiding this comment.
Just figured it helped avoid checking for errors via Write() which were known-impossible, but not worth disrupting the larger picture just for that (and in a previous, unsubmitted version, I was calling it from more than one site, so it was a mild ergonomic improvement).
| // AddReflected uses reflection to serialize arbitrary objects, so it's slow | ||
| // and allocation-heavy. | ||
| // AddReflected uses reflection to serialize arbitrary objects, so it can be | ||
| // slow and allocation-heavy. |
| enc.addKey(key) | ||
| _, err = enc.buf.Write(enc.reflectBuf.Bytes()) | ||
| return err | ||
| enc.buf.AppendBytes(valueBytes) |
There was a problem hiding this comment.
Use of Write here should be fine
There was a problem hiding this comment.
LGTM besides lack of AppendReflected support.
CC @prashantv
| // Only invoke the standard JSON encoder if there is actually something to | ||
| // encode; otherwise write JSON null literal directly. For now, we only | ||
| // apply this optimization here, but not in AppendReflected. We could apply | ||
| // it there as well if needed (some discussion in |
There was a problem hiding this comment.
Since we're treating this as an optimization, I'm in favor of doin it to AppendReflected too.
It already supports nil entries inside slices. https://play.golang.org/p/bZb6t58qhpy
There was a problem hiding this comment.
LGTM, thanks for the improvement!
| // Only invoke the standard JSON encoder if there is actually something to | ||
| // encode; otherwise write JSON null literal directly. For now, we only | ||
| // apply this optimization here, but not in AppendReflected. We could apply | ||
| // it there as well if needed (some discussion in |
This prepares release v1.13.0 with #758 in it.
This prepares release v1.13.0 with #758 in it.
This adds support for building fields from pointers to primitive types we already accept. These new field constructors accept and handle `nil` values in addition to the non-nil primitive values. For nil values, we fall back to the behavior of `zap.Reflect(..)`, which we optimize for in the JSON encoder so that we don't use `encoding/json` just to build the string `"null"`. Resolves uber-go#753
This prepares release v1.13.0 with uber-go#758 in it.
Fixes #753 by exporting util functions to handle nillable primitives. The version here uses
Reflect()under the hood to encodenil, and makes corresponding changes injson_encoder.goto efficiently handle encodingnilfield values.