Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Add iOS 15 payload additions #185
Add iOS 15 payload additions #185
Changes from 4 commits
2a448ab
abd4bb3
6090eba
2b6c257
b7634f4
5485b9d
7133c73
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason to use these four different functions for setting this value rather than a single setter (i.e.
InterruptionLevel
) and define a custom string type alaEPushType
with the values as defined as constants?Definitely appreciate the initiative in getting this out.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I used the four functions as I was following the way that individual functions were provided for badges eg.
ZeroBadge
etc.I am happy to swap to a single setter (which was my original thought tbh).
Welcome your thoughts @chrishaines?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure but it seems to be that this is partially due to typing (it allows
Badge
to specifically take anint
as an argument withUnsetBadge
being used to set it tonil
to remove it from the payload. I'm not sure why there is a separateZeroBadge
function rather than just usingBadge(0)
🤷♂️ ).I think that
EPushType
seems to be the closest precedent to handling an enumerated value in the codebase.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am inclined to swap it out then as you suggest (I have working code for that implementation ready to go).
Before I do though, can I ask, what is the reason for the
E
prefix inEPushType
? Is there some specific naming convention? (I don't do a huge lot in Go).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure where that came from either to be honest. Perhaps it's for
Enum
? That's really a complete guess.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that seems a sensible guess!
Just wondering if I should employ the same
E
prefix or not!There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that sounds reasonable. I would go for it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have swapped out the InterruptionLevel into a single function. Thanks for your input @chrishaines. Please can you give it a once over?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't appear to allow setting a
relevance-score
of0
because it will omitted when building theaps
JSON. This will result in Apple using their default of0.5
. I think you can address this by either using theBadge
precedent above (preferred) or by changing the type to a pointer.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can make a PR for this if you'd like.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch @chrishaines on relevance-score.
Sure if you have time to do the PR for that, and I will try and get the other swapped out tomorrow or at the weekend.
Thanks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here ya go!
neilmorton#1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you @chrishaines! I have merged this.