-
-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
Support incrementing push badge value by more than 1 #4889
Conversation
Codecov Report
@@ Coverage Diff @@
## master #4889 +/- ##
==========================================
+ Coverage 92.85% 92.88% +0.02%
==========================================
Files 119 119
Lines 8821 8828 +7
==========================================
+ Hits 8191 8200 +9
+ Misses 630 628 -2
Continue to review full report at Codecov.
|
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.
Can we make it an object instead of a string to parse? badge: { increment: 3 }
…than string (IncrementBy3)
src/Controllers/PushController.js
Outdated
parseInt(badge.substring(incrementby.length), 10) > 0) { | ||
restUpdate = { badge: { __op: 'Increment', amount: parseInt(badge.substring(incrementby.length), 10) } } | ||
} else if (typeof badge == 'object' && 'increment' in badge && Number(badge.increment)) { | ||
restUpdate = { badge: { __op: 'Increment', amount: badge.increment } } |
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.
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.
If you're talking about using it but only supporting the "increment" case, I'm happy to make the changes.
If you mean more broadly supporting all the same operations, I don't understand all the ramifications well enough myself to be confident in implementing that (I'm fairly new to the code base and likely don't have the time right now to get up to speed on all those different flows)
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.
Yeah, for now that would be just for the increment. Sorry for the back and forths. I believe if we use the same JSON based objects as the rest of the API we’ll be happier on the long term
Awesome! Thanks for the PR and sticking with the change requests :) |
* Support 'IncrementByN' badge value for higher push badge increments * Fix test * Rely on object for badge incrementation (i.e. {increment: 3}) rather than string (IncrementBy3) * For badge incrementation, utilize format similar to other operation notation
* Support 'IncrementByN' badge value for higher push badge increments * Fix test * Rely on object for badge incrementation (i.e. {increment: 3}) rather than string (IncrementBy3) * For badge incrementation, utilize format similar to other operation notation
…#4889) * Support 'IncrementByN' badge value for higher push badge increments * Fix test * Rely on object for badge incrementation (i.e. {increment: 3}) rather than string (IncrementBy3) * For badge incrementation, utilize format similar to other operation notation
No description provided.