Allow ability to explicitly set APNS priority #437
Conversation
| notification.Priority = apns2.PriorityLow | ||
| } else if req.Priority == "high" { | ||
| notification.Priority = apns2.PriorityHigh | ||
| } |
There was a problem hiding this comment.
Why we need another condition req.Priority == "high"?
You can set any value or ignore the property if you want the priority to high.
There was a problem hiding this comment.
Sorry only seeing your comment now, I must have missed it.
You are correct in that you could get away without the second condition. However I just prefer to be explicit about such things. Im not a fan of having API's "assume" things, hence why Id like to be explicit with high priority in the first place.
For example If a dev was to mistakenly put "medium" priority into the request, it would come out as "high", this could be confusing. Ideally in this instance gorush would return an error. The 2nd best option is to not set the property at all (and rely on the implicit default of APS), The third, would be to fall through and set the priority as high. IMO.
This also has the advantage of being "backwards compatible", in that people who never passed an explicit priority wont now be explicitly passing "high" to APS.
Im happy to change it, if its gonna block the PR.
Codecov Report
@@ Coverage Diff @@
## master #437 +/- ##
==========================================
- Coverage 82.95% 80.81% -2.14%
==========================================
Files 20 20
Lines 1396 1423 +27
==========================================
- Hits 1158 1150 -8
- Misses 189 220 +31
- Partials 49 53 +4
Continue to review full report at Codecov.
|
While APNS apparently defaults to high priority https://developer.apple.com/documentation/usernotifications/setting_up_a_remote_notification_server/sending_notification_requests_to_apns.
We have seen behaviour which may contradict this. As such we would like to be able to explicitly set the priority level.