add Encoder and Decoder #59
Conversation
A port of ghodss#39
|
Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please follow instructions at https://git.k8s.io/community/CLA.md#the-contributor-license-agreement to sign the CLA. It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
k8s-ci-robot
added
the
cncf-cla: no
|
Welcome @kevin-hanselman! |
k8s-ci-robot
added
sig/api-machinery
k8s-ci-robot
added
cncf-cla: yes
|
Hey @kevin-hanselman thanks for doing this. Could i persuade you to consider adding some benchmarks? (the linked PR hints at "memory footprint" gains ... so |
|
Added a (very) basic benchmark test. Here's the results on my machine: This branch: master: I'll need to think more about these results and code before drawing any conclusions. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: kevin-hanselman The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
@dims Anything else I can do to move this work along? |
| var intermediate interface{} | ||
| jsonEnc := json.NewEncoder(pipeWriter) | ||
| // We are using yaml.Decoder here (instead of json.Decoder) because the Go | ||
| // JSON library doesn't try to pick the right number type (int, float, |
There was a problem hiding this comment.
json.NewDecoder(...).UseNumber() will preserve numeric input literally (https://pkg.go.dev/encoding/json#Decoder.UseNumber) ... would that enable translating with fidelity? how does the json decoder compare to the yaml decoder in efficiency?
There was a problem hiding this comment.
also, a unit test demonstrating some of the edge cases like this would be helpful in ensuring this isn't producing user-visible changes
other examples of weird edges in yaml encoding/decoding we don't want to accidentally modify behavior on (even if the current behavior is not ultimately desired/correct):
- unquoted yaml keys of
y: CRD definition is changing the property name 'y' in openAPIV3Schema spec to True kubernetes/kubernetes#102308 - yaml parsing of unquoted values containing all digits starting with 0 which are not octal values: Kubernetes unnecessarily requiring quoting on numeric env vars kubernetes/kubernetes#82296
| yamlEnc := yaml.NewEncoder(e.w) | ||
|
|
||
| var jsonErr error | ||
| go func() { |
There was a problem hiding this comment.
spawning goroutines here and in Decode is a little hard to reason about... a few things I noticed/wondered
- if this panics, the calling program will exit instead of propagating the panic to the Encode() caller... I'd expect the panic to propagate instead
- is it better to start the json encoder async and the yaml decoder inline or vice-versa?
- is it better to leave the pipereader unbuffered or wrap in a buffer?
There was a problem hiding this comment.
FWIW, this is adapted from the official io.Pipe docs.
| pipeWriter.Close() | ||
| }() | ||
| yamlErr := yamlDec.Decode(&intermediate) | ||
| if jsonErr != nil { |
There was a problem hiding this comment.
I'm surprised this doesn't complain about races the same way
| var intermediate interface{} | ||
| jsonEnc := json.NewEncoder(pipeWriter) | ||
| // We are using yaml.Decoder here (instead of json.Decoder) because the Go | ||
| // JSON library doesn't try to pick the right number type (int, float, |
There was a problem hiding this comment.
also, a unit test demonstrating some of the edge cases like this would be helpful in ensuring this isn't producing user-visible changes
other examples of weird edges in yaml encoding/decoding we don't want to accidentally modify behavior on (even if the current behavior is not ultimately desired/correct):
- unquoted yaml keys of
y: CRD definition is changing the property name 'y' in openAPIV3Schema spec to True kubernetes/kubernetes#102308 - yaml parsing of unquoted values containing all digits starting with 0 which are not octal values: Kubernetes unnecessarily requiring quoting on numeric env vars kubernetes/kubernetes#82296
| // etc.) when unmarshalling to interface{}; it always picks float64. | ||
| // go-yaml preserves the number type. | ||
| yamlDec := yaml.NewDecoder(pipeReader) | ||
| yamlEnc := yaml.NewEncoder(e.w) |
There was a problem hiding this comment.
This is doing an encode/decode/encode? Why? There has to be a better way than this? There should be some comment explaining the purpose?
|
On Tue, Sep 7, 2021, 16:11 Daniel Smith ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In yaml.go
<#59 (comment)>:
> +}
+
+// Encode serializes a value into YAML and writes it using the Encoder's
+// io.Writer.
+func (e *Encoder) Encode(o interface{}) error {
+ pipeReader, pipeWriter := io.Pipe()
+ defer pipeReader.Close()
+
+ var intermediate interface{}
+ jsonEnc := json.NewEncoder(pipeWriter)
+ // We are using yaml.Decoder here (instead of json.Decoder) because the Go
+ // JSON library doesn't try to pick the right number type (int, float,
+ // etc.) when unmarshalling to interface{}; it always picks float64.
+ // go-yaml preserves the number type.
+ yamlDec := yaml.NewDecoder(pipeReader)
+ yamlEnc := yaml.NewEncoder(e.w)
This is doing an encode/decode/encode? Why? There has to be a better way
than this? There should be some comment explaining the purpose?
Better is a fuzzy metric. I do not use this package in any performance
critical paths, it got the job done, and I have never had to touch it again.
—
… You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#59 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAF4EEXZBEZTWVREDX6D3NDUAZ5W3ANCNFSM5CFGWM3Q>
.
Triage notifications on the go with GitHub Mobile for iOS
<https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675>
or Android
<https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.
|
|
Cross-referencing #61 (comment) with information about potential blockers / info to think about with regards to this PR. |
k8s-ci-robot
added
the
needs-rebase
|
@kevin-hanselman: PR needs rebase. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
/uncc |
|
The Kubernetes project currently lacks enough contributors to adequately respond to all issues and PRs. This bot triages issues and PRs according to the following rules:
You can:
Please send feedback to sig-contributor-experience at kubernetes/community. /lifecycle stale |
k8s-ci-robot
added
the
lifecycle/stale
|
I currently do not have the bandwidth to push this PR forward. I'll close this PR until further notice, and I welcome someone else becoming its sponsor. |
A port of ghodss#39
Original code by @VictorLowther, adapted to this fork by me.