⚠️ Add support for encoding.TextMarshaler #1015
Conversation
k8s-ci-robot
added
cncf-cla: yes
|
Hi @twz123. Thanks for your PR. I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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-sigs/prow repository. |
k8s-ci-robot
added
the
size/M
| // URL4 is an alias of [net/url.URL]. It implements [encoding.TextMarshaler] so | ||
| // that it can be used in K8s CRDs such that the CRD resource will have the URL | ||
| // but operator code can can work with the URL struct. | ||
| type URL4 url.URL |
There was a problem hiding this comment.
I do not understand the difference between the two, could you explain why they both exist?
What seems to be missing as a testcase is something that implements both json marshaller and text marshaller, in that case the json marshalling must take precedence
There was a problem hiding this comment.
I do not understand the difference between the two, could you explain why they both exist?
It's been a while since I wrote that piece of code. Need to figure out myself 🙃
What seems to be missing as a testcase is something that implements both json marshaller and text marshaller, in that case the json marshalling must take precedence
Will do a rebase and add the test in the next few days I hope.
There was a problem hiding this comment.
I do not understand the difference between the two, could you explain why they both exist?
You mean URL3 and URL4? I mimicked the URL and URL2 types. One is a regular struct, the other is a newtype. I guess it makes sense to have both, as they might behave differently when using reflection?
What seems to be missing as a testcase is something that implements both json marshaller and text marshaller, in that case the json marshalling must take precedence
Added.
k8s-ci-robot
added
ok-to-test
Whenever a type is encountered that implements encoding.TextMarshaler but not encoding/json.Marshaler, assume that it will be encoded as a string. This is a breaking change, as types that implement TextMarshaler are now handled differently. On the other hand, this probably restores reality for all but the most customized setups, as Go's JSON package will also check for the presence of this interface, and output a JSON string. Signed-off-by: Tom Wieczorek <[email protected]>
Test that json.Marshaler has precedence over encoding.TextMarshaler. Signed-off-by: Tom Wieczorek <[email protected]>
twz123
force-pushed
the
textmarshaler-support
branch
from
5462dfc
to
768681b
Compare
k8s-ci-robot
added
size/L
alvaroaleman
added
the
tide/merge-method-squash
k8s-ci-robot
added
the
lgtm
|
LGTM label has been added. Git tree hash: 601d830a18f14ade1eee173d2ecf80e4af823d80
|
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: alvaroaleman, twz123 The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
approved
Whenever a type is encountered that implements
encoding.TextMarshalerbut notencoding/json.Marshaler, assume that it will be encoded as a string.This is a breaking change, as types that implement
TextMarshalerare now handled differently. On the other hand, this probably restores reality for all but the most customized setups, as Go's JSON package will also check for the presence of this interface, and output a JSON string.