Depointerize maps and slices

[timoreimann]

As the zero value for a reference type is nil and becomes empty upon initialization, we do not need to use pointers for maps and structs when it comes to distinguishing omitted and empty JSON values.

Other drive-by refactorings include:

• Rename Empty{Label,Env} to Empty{labels,Envs} for consistency reasons.
• Compare length with == instead of <=.
• Fix in-code typos.
• Minor cosmetics.

Closes #137.

[timoreimann]

@bobrik, would you mind reviewing?

@kesselborn, feel free to also have a look.

[bobrik]

LGTM

[gambol99]

👍 LGTM

[lamdor]

@timoreimann @gambol99 I ran into a problem that this PR introduced. With it no longer being a pointer, there's no way to force it to send an empty array as the json annotations have omitempty. When it's set to an empty array, it won't be added it to the JSON and marathon won't update the value. For example: https://play.golang.org/p/PlCkLSaQJu, when I change the Args from a pointer, to a straight array, json won't include that field.

I ran into this because I had an application with Env set, but when I tried to set it to make([]string, 0), it wasn't getting changed in marathon.

[bobrik]

@rubbish you probably want mesosphere/marathon#1418

[lamdor]

@bobrik But this isn't about setting it to null, it's about setting it to an empty array. This PR changed it so I can't update an applications' Args from an array with values to an empty array. This is because of the omitempty will not send it as args: [], but won't marshall it at all.

[timoreimann]

What I didn't realize when I put together the PR is that initialized, empty reference types apparently still count as zero values from encoding/json's perspective and thus are being omitted when using omitempty. This basically kills the ability to clear out fields in the Marathon API, which is the opposite of what I wanted to achieve. :(

I believe we should revert the change. To address @bobrik's original concern, we can consider providing accessor methods to make working with pointers more convenient. That should be subject for a different issue, though.

Thoughts?

[bobrik]

@rubbish null and empty array should have the same meaning for PUT (as opposed to PATCH), but marathon does not implement that yet.

@timoreimann what about dropping omitempty? http://play.golang.org/p/Ua7DV8Lge1

[timoreimann]

@bobrik Dropping omitempty is not an option from my perspective as it would cause fields not explicitly set to something other than empty to be marshaled into empty (non-omitted) values by default. This makes it very easy to unintentionally remove/clear JSON properties unless the consumer is extremely careful.

See also this blog post that served as a basis for the PR which introduced pointers in go-marathon in the first place. We basically leaned towards the same argumentation at the time.

[timoreimann]

Okay looking at your playground example, dropping omitempty might actually be reasonable for reference types...

[lamdor]

👍 for removing omitempty. It's been troublesome for me in the past.

[timoreimann]

On a closer look, the serialized outcome of an uninitialized reference type missing the omitempty tag is not identical to an uninitialized pointerized reference type including the omitempty tag: Have a look at https://play.golang.org/p/oR5HKhWRIs and notice how the former is being marshaled into {"Things:" null} while the latter is being omitted.

Even if Marathon treated null-mapped properties equally to omitted ones now, I'd feel anxious to rely on this assumption in the future. Thus, my preference goes towards the less wieldy but more safe pointerized version.

[bobrik]

Yeah, this makes sense. I wonder what's the idiomatic way.

[timoreimann]

@bobrik I have the feeling that there's no clear idiomatic way. FWIW though, the author of go-github (who wrote the blog post I linked above) believes that the pointer approach is the most idiomatic one. See also his comment.

Preparing a reverting PR now.

[timoreimann]

Things should be fine again hopefully. If not, please let me know.

Apologies for the mess. Beyond simple cases, encoding/json isn't the easiest to work this. 😕