-
Notifications
You must be signed in to change notification settings - Fork 739
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
Convert PushRuleService to suspend functions #2414
Conversation
Signed-off-by: Dominic Fischer <[email protected]>
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, 2 small remarks though
} | ||
.executeBy(taskExecutor) | ||
override suspend fun removePushRule(kind: RuleKind, pushRule: PushRule) { | ||
return removePushRuleTask.execute(RemovePushRuleTask.Params(kind, pushRule)) |
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.
In all those 4 methods, the return
statement could be removed.
result.onSuccess { | ||
preference.setPushRule(ruleAndKind.copy(pushRule = newRule)) | ||
} | ||
hideLoadingView() |
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 is a detail, but now, I would rather call hideLoadingView()
just after the check if Fragment is added.
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.
Technically the behaviour would be different (out of scope) but I'm not bothered if you're not bothered.
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.
Yes. I do not think it will introduce a bug if we reorder. Also we could use .fold()
instead of onSuccess
and onFailure
which is a bit more verbose (but equivalent)
Signed-off-by: Dominic Fischer <[email protected]>
Signed-off-by: Dominic Fischer <[email protected]>
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.
Thanks for the update
Signed-off-by: Dominic Fischer [email protected]
Pull Request Checklist