Skip to content
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

Treat EDN nil as Go zero value #11

Open
tolitius opened this issue Nov 3, 2017 · 5 comments
Open

Treat EDN nil as Go zero value #11

tolitius opened this issue Nov 3, 2017 · 5 comments

Comments

@tolitius
Copy link
Contributor

tolitius commented Nov 3, 2017

some systems that I don't control could send:

{:a nil :b 34}

which would result in:

edn: cannot unmarshal nil into Go value of type string.

it would make sense to convert EDN nils to a Go type zero values, or simply drop them. Then we can either not worry about it or use ",omitempty" to ignore them.

What do you think?

@tolitius
Copy link
Contributor Author

tolitius commented Nov 3, 2017

i.e. why not:

v.Set(reflect.Zero(v.Type()))

here:

edn/decode.go

Lines 910 to 912 in 470b0ab

default:
d.error(&UnmarshalTypeError{"nil", v.Type()})
}

?

@hypirion
Copy link
Member

hypirion commented Nov 3, 2017

It depends on how you need to treat nils. Are they equal to the zero value in your case, or are they identical to an absence of value?

As a short term solution, you can use

type Good struct {
  A *string
  B int
}

instead of

type Error struct {
  A string
  B int
}

And also get information about whether this is a nil value or not.


So in general, I think there's value in distinguishing the between the zero value ("", 0, etc) nil, and the absence of a value. Consider the following patch bodies:

;; 1
{:author #person/id 0}
;; 2
{:author nil}
;; 3
{}

Typical behaviour is that the first one sets the author to the zero value, the second sets the author to null, and the third does not update the author field.

I'm not against nil being equal to the zero value or the absence of a value, but I don't think I'd like it to be the default. Maybe a new option ignore-nil or nil-as-zero or something? I need to think about what makes the most sense here, as ignoring the value is different from setting it to the zero value.

@tolitius
Copy link
Contributor Author

tolitius commented Nov 3, 2017

there's value in distinguishing the between the zero value ("", 0, etc) nil, and the absence of a value

I agree, although (talking about unmarshalling EDN) I feel that EDN's nil (on a Clojure side) most of the time means absence of the value. i.e. {:author #person/id 0} does not mean nil in Clojure.

Maybe a new option ignore-nil or nil-as-zero or something?

I think both may have a place.

If you decide to leave the current behavior as a default one (which you are leaning to) it would help to have a mechanism to define ignore-nil or nil-as-zero on a struct level, otherwise it would go back to required vs. optional protobuf (2.x) problem. (in protobuf 3 fields are optional by default).

tolitius added a commit to tolitius/go-edn that referenced this issue Nov 3, 2017
@tolitius
Copy link
Contributor Author

tolitius commented Nov 3, 2017

[the commit above]: me trying things, i.e. no intention to change the official repo

Rhetorical observation:

	wantptr := ttype == tokenSymbol && bytes.Equal(nilByte, bs)

When unmarshalling from EDN I don't think nilByte (i.e. EDN's nil) means "want a Go pointer", but rather "nothing" for whatever type.

@hypirion
Copy link
Member

hypirion commented Nov 3, 2017

Yeah, the idea is to have ignore-nil or what we decide to call it a property in struct tags, e.g.

type Foo struct {
    Bar string `edn:",ignore-nil"
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants