-
Notifications
You must be signed in to change notification settings - Fork 3
feat: add direct APNS support #187
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
Conversation
1018022 to
8cebff7
Compare
e2f1de0 to
2afd3ba
Compare
92706dc to
a338e54
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #187 +/- ##
==========================================
+ Coverage 87.73% 89.25% +1.52%
==========================================
Files 8 9 +1
Lines 318 391 +73
==========================================
+ Hits 279 349 +70
- Misses 39 42 +3
Continue to review full report in Codecov by Sentry.
|
676767c to
ee11028
Compare
ee11028 to
0f28af3
Compare
pierre-1997
left a comment
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.
Mostly nitts and details
0f28af3 to
03ff3c6
Compare
pierre-1997
left a comment
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.
A few additional stuff
03ff3c6 to
d7c89b0
Compare
pierre-1997
left a comment
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.
You can probably avoid the .clone() call on dev.notify_via.clone().unwrap_or_default() btw and then match &NotificationMethod::XXX (I think, not sure about this) but that's not that costly anyways so I'm just approving :D Feel free to ping me if you change it and need a reapproval !
No description provided.