Skip to content
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

Merged
merged 3 commits into from Nov 19, 2020

Conversation

Dominaezzz
Copy link
Contributor

Signed-off-by: Dominic Fischer dominicfischer7@gmail.com

Pull Request Checklist

  • Changes has been tested on an Android device or Android emulator with API 21
  • UI change has been tested on both light and dark themes
  • Pull request is based on the develop branch
  • Pull request updates CHANGES.md
  • Pull request includes screenshots or videos if containing UI changes
  • Pull request includes a sign off

Signed-off-by: Dominic Fischer <dominicfischer7@gmail.com>
Copy link
Member

@bmarty bmarty left a 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))
Copy link
Member

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()
Copy link
Member

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.

Copy link
Contributor Author

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.

Copy link
Member

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 <dominicfischer7@gmail.com>
Signed-off-by: Dominic Fischer <dominicfischer7@gmail.com>
Copy link
Member

@bmarty bmarty left a 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

@bmarty bmarty merged commit d5f83ac into element-hq:develop Nov 19, 2020
@Dominaezzz Dominaezzz deleted the suspend_functions_2 branch November 19, 2020 22:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants