FAQ
PTAL

The code now ignores unexported fields completely as requested in the
issue discussion.

I left the UnmarshalFieldError and noted that it is deprecated in the
comments since removing it could break existing code. Do you think
that's the correct approach?

Thanks.


https://codereview.appspot.com/7139049/

Search Discussions

  • Rsc at Jan 18, 2013 at 4:33 pm
    LGTM

    Okay I guess. Would like to hear from Brad too.



    https://codereview.appspot.com/7139049/diff/13001/src/pkg/encoding/json/decode.go
    File src/pkg/encoding/json/decode.go (right):

    https://codereview.appspot.com/7139049/diff/13001/src/pkg/encoding/json/decode.go#newcode109
    src/pkg/encoding/json/decode.go:109: // (Deprecated; kept for
    compatibility.)
    (No longer used; kept for compatibility.)

    https://codereview.appspot.com/7139049/
  • Rickarnoldjr at Jan 18, 2013 at 5:33 pm
    PTAL



    https://codereview.appspot.com/7139049/diff/13001/src/pkg/encoding/json/decode.go
    File src/pkg/encoding/json/decode.go (right):

    https://codereview.appspot.com/7139049/diff/13001/src/pkg/encoding/json/decode.go#newcode109
    src/pkg/encoding/json/decode.go:109: // (Deprecated; kept for
    compatibility.)
    On 2013/01/18 16:33:15, rsc wrote:
    (No longer used; kept for compatibility.)
    Done.

    https://codereview.appspot.com/7139049/
  • Bradfitz at Jan 18, 2013 at 6:47 pm
    LGTM

    I didn't expect my little regression bug report to result in this much
    of a change, but I'm fine with it. I found the old behavior too scary.

    If we wanted to keep UnmarshalFieldError for pedagogical purposes,
    perhaps we only return it when the Unmarshal would otherwise decode no
    fields.

    So a new user doing:

    type T struct {
    foo string
    }

    and trying to decode `{"foo": "bar"}` would get the error, but NOT a
    user trying to decode:

    type T struct {
    Name string `json:"name"`
    foo string
    }

    ... trying to decode `{"foo": "bar", "name": "Bob"}`

    because in that case, some progress was made.

    I haven't thought through the negative consequences of that too much,
    though, and I imagine people could just read the docs or show up on the
    mailing list with questions.


    https://codereview.appspot.com/7139049/
  • Rickarnoldjr at Jan 18, 2013 at 7:06 pm

    On 2013/01/18 18:47:48, bradfitz wrote:
    LGTM
    I didn't expect my little regression bug report to result in this much of a
    change, but I'm fine with it. I found the old behavior too scary.
    If we wanted to keep UnmarshalFieldError for pedagogical purposes,
    perhaps we
    only return it when the Unmarshal would otherwise decode no fields.
    So a new user doing:
    type T struct {
    foo string
    }
    and trying to decode `{"foo": "bar"}` would get the error, but NOT a
    user trying
    to decode:
    type T struct {
    Name string `json:"name"`
    foo string
    }
    ... trying to decode `{"foo": "bar", "name": "Bob"}`
    because in that case, some progress was made.
    I haven't thought through the negative consequences of that too much, though,
    and I imagine people could just read the docs or show up on the
    mailing list
    with questions.
    I did notice that there is a somewhat related issue on documentation:
    4664. Seems like Unmarshal field mapping is a common area of confusion.
    Maybe along with that issue we can document that non-exported fields are
    always ignored?


    https://codereview.appspot.com/7139049/
  • Russ Cox at Jan 18, 2013 at 7:12 pm
    I don't want to special-case zero fields. It's too fussy.

    Russ
  • Andrew Gerrand at Jan 18, 2013 at 11:13 pm

    On 19 January 2013 06:12, Russ Cox wrote:

    I don't want to special-case zero fields. It's too fussy.

    Are you sure? I think it makes sense to give an error if the user provides
    a type that is unusable by encoding/json (no exported fields or
    UnmarshalJSON method).
  • Brad Fitzpatrick at Jan 19, 2013 at 4:46 pm
    On Fri, Jan 18, 2013 at 3:13 PM, Andrew Gerrand wrote:
    On 19 January 2013 06:12, Russ Cox wrote:

    I don't want to special-case zero fields. It's too fussy.

    Are you sure? I think it makes sense to give an error if the user provides
    a type that is unusable by encoding/json (no exported fields or
    UnmarshalJSON method).
    Oh, I hadn't even thought of that. That's much better than what I was
    proposing.

    Yes, if the user pass *T to Unmarshal but T is:

    type T struct {
    onlyLower string
    etc int
    }

    ... there's no way it'll ever work. That seems like a good place to teach.
  • Rick Arnold at Jan 21, 2013 at 9:27 pm
    Russ, are you ok with this approach?

    Thanks.

    On Saturday, January 19, 2013 11:46:39 AM UTC-5, Brad Fitzpatrick wrote:



    On Fri, Jan 18, 2013 at 3:13 PM, Andrew Gerrand <a...@golang.org<javascript:>
    wrote:
    On 19 January 2013 06:12, Russ Cox <r...@golang.org <javascript:>> wrote:

    I don't want to special-case zero fields. It's too fussy.

    Are you sure? I think it makes sense to give an error if the user
    provides a type that is unusable by encoding/json (no exported fields or
    UnmarshalJSON method).
    Oh, I hadn't even thought of that. That's much better than what I was
    proposing.

    Yes, if the user pass *T to Unmarshal but T is:

    type T struct {
    onlyLower string
    etc int
    }

    ... there's no way it'll ever work. That seems like a good place to teach.

  • Andrew Gerrand at Jan 18, 2013 at 11:12 pm

    On 19 January 2013 06:06, wrote:

    Maybe along with that issue we can document that non-exported fields are
    always ignored?
    Yes, the docs need updating.

Related Discussions

Discussion Navigation
viewthread | post
Discussion Overview
groupgolang-dev @
categoriesgo
postedJan 18, '13 at 2:24a
activeJan 21, '13 at 9:27p
posts10
users4
websitegolang.org

People

Translate

site design / logo © 2022 Grokbase