-
Notifications
You must be signed in to change notification settings - Fork 57
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
fix: adjust increment api and remove add from NumberSignal #2704
fix: adjust increment api and remove add from NumberSignal #2704
Conversation
This will introduce increment event for NumberSignal so that it is processed atomically on server based on the last seen value of NumberSignal without any retries. Also, this refactors the increment method to incrementBy, and also removes the add methods that were introduced in #2694
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2704 +/- ##
==========================================
- Coverage 94.35% 94.34% -0.02%
==========================================
Files 80 80
Lines 2551 2545 -6
Branches 661 658 -3
==========================================
- Hits 2407 2401 -6
Misses 140 140
Partials 4 4
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
I wonder what's the problem with |
Quality Gate passedIssues Measures |
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.
LGTM! Just two minor things to report, feel free to ignore.
@@ -134,7 +134,7 @@ public void constructor_withJsonInvalidEventType_shouldThrowInvalidEventTypeExce | |||
StateEvent.InvalidEventTypeException.class, | |||
() -> new StateEvent<>(json, String.class)); | |||
|
|||
String expectedMessage = "Invalid event type invalidType. Type should be either of: [SNAPSHOT, SET, REPLACE, REJECT]"; | |||
String expectedMessage = "Invalid event type invalidType. Type should be either of: [SNAPSHOT, SET, REPLACE, REJECT, 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.
"either of" is rather used with two options. Should be "one of".
|
||
private ObjectNode createIncrementEvent(String value) { | ||
var setEvent = new StateEvent<>(UUID.randomUUID().toString(), | ||
StateEvent.EventType.INCREMENT, Double.parseDouble(value)); |
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.
valueOf
is slightly better than parseDouble
here (IDE suggestion)
Sorry @platosha the fact that I requested a new review from you is an unwanted mouse click. |
This ticket/PR has been released with Hilla 24.5.0.alpha15 and is also targeting the upcoming stable 24.5.0 version. |
Description
This will introduce increment event for NumberSignal so that it is processed atomically on server based on the last seen value of NumberSignal without any retries.
Also, this refactors the increment method to incrementBy, and also removes the add methods that were introduced in #2694