Sync polls' push rules (PSG-77, PSG-1097) #7320
Conversation
alfogrillo
requested review from
a team and
Anderas
and removed request for
a team
Codecov ReportBase: 12.02% // Head: 12.01% // Decreases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## develop #7320 +/- ##
===========================================
- Coverage 12.02% 12.01% -0.01%
===========================================
Files 1630 1631 +1
Lines 161272 161473 +201
Branches 66073 66252 +179
===========================================
+ Hits 19391 19404 +13
- Misses 141237 141420 +183
- Partials 644 649 +5
Flags with carried forward coverage won't be shown. Click here to find out more.
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
alfogrillo
force-pushed
the
alfogrillo/sync_polls_rules
branch
from
c98da16
to
731b343
Compare
alfogrillo
changed the title
...SwiftUI/Modules/Settings/Notifications/Service/MatrixSDK/MXNotificationSettingsService.swift
Outdated
Show resolved
Hide resolved
| private extension MXNotificationSettingsService { | ||
| func enableRule(rule: MXPushRule, enabled: Bool, completion: ((Result<Void, Error>) -> Void)?) { | ||
| session.notificationCenter.enableRule(rule, isEnabled: enabled) { error in | ||
| completion?(error.result) |
There was a problem hiding this comment.
Hm this looks a bit confusing. If I understand correctly error.result will either give you error or success, but the reader would not know this without looking at the implementation of the result extension. Being explicit via some if let/else may mean more lines of code but is in my opinion clearer.
There was a problem hiding this comment.
Yeah it's just converting an objective-c style Error? to a swifty Result<Void, Error>.
I'll prefer to keep the extension because I need it several times.
There was a problem hiding this comment.
Isn't it just this?
if let error = error {
completion(.failure(error)
} else {
completion(.success(()))
}You do use error.result several times but with similar level of ambiguity, see this:
switch error.result {
case .success:
self?.enableRule(rule: rule, enabled: enabled, completion: completion)
case .failure:
completion?(error.result)
}This is even longer than doing the if/else and we have to unpack error.result twice so it really looks like the wrong kind of API to me
| @@ -40,5 +40,16 @@ protocol NotificationSettingsServiceType { | |||
| /// - ruleId: The id of the rule. | |||
| /// - enabled: Whether the rule should be enabled or disabled. | |||
| /// - actions: The actions to update with. | |||
| func updatePushRuleActions(for ruleId: String, enabled: Bool, actions: NotificationActions?) | |||
| /// - completion: The completion of the operation. | |||
| func updatePushRuleActions(for ruleId: String, enabled: Bool, actions: NotificationActions?, completion: ((Result<Void, Error>) -> Void)?) | |||
There was a problem hiding this comment.
As a speculative thought what would happen if we made this method async throws and having the
completion variant (which is used less often and only with obj-c) the extension if even necessary? I think it would make the code more readable. It also seems that you have already added async code elsewhere in this PR
There was a problem hiding this comment.
I think it should work as well.
I preferred to keep the non async version in the first place to minimize the refactor I needed to do.
But at this point it should be an easy one.
There was a problem hiding this comment.
I think it may also help with some of the Result<Void, Error> workarounds and magic
There was a problem hiding this comment.
I refactored several things trying to leverage more async/await to make the code easier to be read.
I also found some already existing memory leak and I fixed that as well.
6850633
alfogrillo
force-pushed
the
alfogrillo/sync_polls_rules
branch
from
9b2214a
to
8d734b9
Compare
|
Kudos, SonarCloud Quality Gate passed!
|
Description
This PR syncs the poll rules with the ones of normal messages.
Dependency
matrix-org/matrix-ios-sdk#1702
Result