timehop / apns

A Go package to interface with the Apple Push Notification Service
https://godoc.org/github.com/timehop/apns
MIT License
185 stars 47 forks source link

What is the reason for APS.MarshalJSON? #28

Closed willfaught closed 9 years ago

willfaught commented 9 years ago

I don't understand why APS.MarshalJSON is defined:

func (aps APS) MarshalJSON() ([]byte, error) {
    data := make(map[string]interface{})

    if !aps.Alert.isZero() {
        data["alert"] = aps.Alert
    }
    if aps.Badge != nil {
        data["badge"] = aps.Badge
    }
    if aps.Sound != "" {
        data["sound"] = aps.Sound
    }
    if aps.ContentAvailable != 0 {
        data["content-available"] = aps.ContentAvailable
    }
    if aps.Category != "" {
        data["category"] = aps.Category
    }
    if aps.URLArgs != nil && len(aps.URLArgs) != 0 {
        data["url-args"] = aps.URLArgs
    }

    return json.Marshal(data)
}

This appears to almost duplicate the default marshaling behavior for json struct tags. The only difference I can see is with Alert, where Alert.Title/Action don't affect whether Alert is included, and I don't understand why. Perhaps I'm missing something?

nathany commented 9 years ago

See #14.

Release notes for the project would would be nice, but since that isn't in place, git blame can be useful for seeing what changes are done and correlating those back to previous pull requests.

willfaught commented 9 years ago

Good tip!

OK, so it was to get around the fact that the Alert field is a value type. I still don't understand why the Alert's title and action aren't included in the zero check. That looks like a bug to me.

nathany commented 9 years ago

You're right, it's probably a bug from working on this across a few separate pull requests.

9 added Title and Action for Safari but apparently I didn't update isZero. :frowning: :palm_tree:

willfaught commented 9 years ago

I've been looking through #9 and the others linked to from there, but I don't see the reason why Alert wasn't made a pointer. There's actually a post by you saying you'd changed it to a pointer. What was the reason ?

nathany commented 9 years ago

That's a good question. Other than breaking the API, though we broke the API to make badge a pointer, so...

taylortrimble commented 9 years ago

Given that the loose coupling of the IsZero command has immediately proven to be fraught with peril, I'd support making Alert a pointer.

nathany commented 9 years ago

Agreed. It's probably a better solution than introducing reflection or dealing with maintaining IsZero.

bdotdub commented 9 years ago

Ditto. Closing in favor of #26 - with the addition of making Alert a pointer.

Feel free to reopen if there is more to discuss.