search

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

Added json tags to APS, formatted #26

Closed willfaught closed 9 years ago

willfaught commented 9 years ago

APS json tags allow unmarshaling APSes from storage.

willfaught commented 9 years ago

This is for when you're storing the APS you want to use later. The json package will still use MarshalJSON. For example:

useraps := ... // input from user
mydatabase.Store("useraps", useraps) // uses MarshalJSON
... // later
useraps := mydatabase.Load("useraps") // uses json tags
n := apns.NewNotification()
n.Payload.APS = useraps
apns.NewClient(...).Send(n) // uses MarshalJSON

Sorry, I didn't know pre-1.4 compatibility was a concern. I just blindly ran gofmt. I can remove the formatting.

willfaught commented 9 years ago

Updated to remove the formatting.

nathany commented 9 years ago

Actually I'm not sure if pre-1.4 compatibility is a concern or not. Just leaving a note.

I should note that @bdotdub is the project owner, I'm just trying to help out.

taylortrimble commented 9 years ago

I don't really support using tags to deserialize and a custom method to serialize. I'd rather those be symmetric; either use tags for both, or create an UnmarshalJSON method to complement MarshalJSON.

If you do tags, we'd have to make Alert a pointer as discussed in #28. That's the route I'd take for this pull request.

willfaught commented 9 years ago

Agreed, it would be better if it were symmetric.

I didn't want to break the API without knowing the project's compatibility policy. I'd rather not write an UnmarshalJSON. I can amend my PR to change Alert to a pointer type if that's acceptable.

taylortrimble commented 9 years ago

Ballsy, but I think there's room for that. :smile:

bdotdub commented 9 years ago

Another :+1: for symmetric. I think having Alert be a pointer makes sense – sounds like we all agree it's more semantically correct.

nathany commented 9 years ago

While we're changing the API for Alert, I should point out the official docs for the Notification Payload also allow alert to be a string.

You may specify a string as the value of alert or a dictionary as its value. If you specify a string, it becomes the message text of an alert.

I'm not sure if that's something we want or need to support, but maybe it's something we should think about.

willfaught commented 9 years ago

@nathany anachronistic/apns uses interface{} for Alert, which seemed reasonable to me.

nathany commented 9 years ago

ok.

taylortrimble commented 9 years ago

I'm a fan of #35's approach.

As for serialization for storage, that's pretty highly app-specific. This could go in databases, a binary format, etc. I would put the individual fields into a postgres db. Also, as fields change, this approach to storage is risky. If we rename a field, recovering the struct from storage would fail. This is why storage should be:

nathany commented 9 years ago

Agree with @tylrtrmbl here. I think you're better off using a JSON struct in your app for serialization, ensuring that it doesn't change when a dependency changes.