-
Notifications
You must be signed in to change notification settings - Fork 181
Add support for JSON encoding nested struct #54
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
Conversation
Codecov Report
@@ Coverage Diff @@ ## master #54 +/- ## =========================================== - Coverage 100.00% 98.23% -1.77% =========================================== Files 1 1 Lines 162 170 +8 =========================================== + Hits 162 167 +5 - Misses 0 2 +2 - Partials 0 1 +1
Continue to review full report at Codecov.
|
So this is a weird option to be sure. This seems pretty obscure, and so I'm a little hesitant to add it to the core library versus justing saying this user a custom encoder. That said, if we did include this, here's some thoughts...
does this need to be limited only to structs? If we're going to support a 'json' tag option, we should probably support it equally across types. So this logic can be moved higher up in the func... see willnorris@b9a5f66 for example. That seems like a reasonable place to put it, but I'm not 100% sure.
I'm not quite sure about the interaction with omitempty. If you have a field tag of url:",json,omitempty"
, and the result of json.Marshal
is an empty string, shouldn't that be omitted? I don't think it would be in my link above, though I haven't tested it.
What is the interaction between a url
tag and json
tag on the same field? Would that be handled correctly? For example...
type T struct { field F `url:"f,json" json:"g,omitempty"` }
What even should happen there? Fortunately, the encoding/json
package doesn't define very many encoding options, so there's not too much to worry with. field names and omitempty are likely the main ones to test.
Speaking of tests, I think the test you have right now is actually more complex than it really needs to be, making it a little harder to understand. Or maybe it's just trying to test too much in each case that's making it hard to read? See TestValues_Slices
for example, where test cases take advantage of anonymous structs... it is more verbose, but makes each test case more self-contained. And if we do expand this to be type agnostic, then we'd want to test a few different types, particularly things that have unique JSON encodings... maps, slices, and structs. Writing readable tests for this package has always been a challenge, so I can also continue to iterate on them later if needed.
So this is a weird option to be sure. This seems pretty obscure, and so I'm a little hesitant to add it to the core library versus justing saying this user a custom encoder. That said, if we did include this, here's some thoughts...
I've been working on an API wrapper for a SaaS called Zoho in my spare time. One of their services uses URL encoded forms, but some of the fields are encoded in JSON. In order to minimize work for the contributor of this endpoint I decided this was the simplest means. Admittedly a late-night weekend hackjob so I really appreciate the thoughtful response.
does this need to be limited only to structs? If we're going to support a 'json' tag option, we should probably support it equally across types. So this logic can be moved higher up in the func... see willnorris@b9a5f66 for example. That seems like a reasonable place to put it, but I'm not 100% sure.
I'm not quite sure about the interaction with omitempty. If you have a field tag of
url:",json,omitempty"
, and the result ofjson.Marshal
is an empty string, shouldn't that be omitted? I don't think it would be in my link above, though I haven't tested it.
No, omitempty wouldn't be observed currently. A totally empty JSON should still be {}
at least I think. Testing the struct for emptiness would require some extra reflection I think, something like reflect.New(sv.Elem()).Interface() == sv.Interface()
.
Testing the returned string/[]byte isn't feasible if the provided struct contains additional nested structs, see below.
What is the interaction between a
url
tag andjson
tag on the same field? Would that be handled correctly? For example...type T struct { field F `url:"f,json" json:"g,omitempty"` }What even should happen there? Fortunately, the
encoding/json
package doesn't define very many encoding options, so there's not too much to worry with. field names and omitempty are likely the main ones to test.
Not sure what should happen. When providing a struct to encoding/json
it will not observe omitempty on a nested struct it seems. So in the snippet above, passing T
to json.Marshal
will always return {"g":{.....}}
and this package would return f={.....}
. Check https://play.golang.org/p/nBS3YrgJhuz.
I can see the concern if a user were to provide a tag like: url:"f,json,omitempty"
and received f={"x":{}}
as the response. But they would/should have that expectation from encoding/json
already, hopefully.
Speaking of tests, I think the test you have right now is actually more complex than it really needs to be, making it a little harder to understand. Or maybe it's just trying to test too much in each case that's making it hard to read? See
TestValues_Slices
for example, where test cases take advantage of anonymous structs... it is more verbose, but makes each test case more self-contained. And if we do expand this to be type agnostic, then we'd want to test a few different types, particularly things that have unique JSON encodings... maps, slices, and structs. Writing readable tests for this package has always been a challenge, so I can also continue to iterate on them later if needed.
I just copy-pasted some other tests to get something that passed. I'll do something nicer if we decide to publish.
I'm happy to work it out anyway you want, or we can abandon this. My use case is rather thin, I've made a fork, and I really hope Zoho updates the endpoints in question some day (there aren't many services like this AFAIK).
If you have a preferred direction I can give it another go, otherwise feel free to close or sit on this PR.
Thanks and best regards,
Schmorrison
Resolves #53