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

Improve usage of format strings #687

merged 4 commits into from Feb 6, 2018


Copy link

@typfel typfel commented Feb 5, 2018

What's new in this PR?


We have some usage of format strings where it is possible to construct incorrect format strings if great care is not taken when using the localization methods.


Issue 1: ZMUserSession+UserNotificationCategories.m
Here we are passing format strings as variables down a chain of methods which can be risky if the key is not used as expected. We can fix this by resolving the localized string at the top level and pass it down instead.

issue 2: ZMLocalNotificationLocalization.h
Here we expose several localization API method as an extension on NSString. These methods however only work with a certain set of localization strings defined in Push.strings. Knowing which methods work together with which localization strings requires careful inspection of the code and is easy to get wrong.

We will solve this replacing these methods with an enum representing every case of possible push notifications. This enum will then expose methods for creating the necessary localized strings which will always return a valid result.

case 0:
return String.localizedStringWithFormat(format)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We still are passing dynamic string to a format here, we should use regular localizedString(forKey: method instead

@@ -304,6 +304,7 @@ extension LocalNotificationDispatcherTests {
XCTAssertEqual(self.application.scheduledLocalNotifications.count, 1)
XCTAssertEqual(self.notificationDelegate.receivedLocalNotifications.count, 0)
guard let notification = self.application.scheduledLocalNotifications.first else { return XCTFail() }
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably needs to go away:)

Copy link

codecov-io commented Feb 5, 2018

Codecov Report

Merging #687 into develop will increase coverage by 0.16%.
The diff coverage is 88.35%.

Impacted file tree graph

@@            Coverage Diff             @@
##           develop    #687      +/-   ##
+ Coverage    82.23%   82.4%   +0.16%     
  Files          173     173              
  Lines        12358   12344      -14     
+ Hits         10163   10172       +9     
+ Misses        2195    2172      -23
Impacted Files Coverage Δ
...Session/ZMUserSession+UserNotificationCategories.m 0% <0%> (ø) ⬆️
Source/Calling/CallKitDelegate.swift 66.19% <100%> (+0.97%) ⬆️
...on Types/LocalNotificationType+Configuration.swift 100% <100%> (ø)
...ification Types/ZMLocalNotification+Messages.swift 98% <100%> (+2%) ⬆️
...tification Types/ZMLocalNotification+Calling.swift 100% <100%> (+4.34%) ⬆️
...tions/Notification Types/ZMLocalNotification.swift 52.94% <28.57%> (-19.06%) ⬇️
...otification Types/ZMLocalNotification+Events.swift 95.17% <90%> (-2.79%) ⬇️
...ification Types/LocalNotificationContentType.swift 76.19% <90.47%> (ø)
...ion Types/LocalNotificationType+Localization.swift 93.71% <93.71%> (ø)
... and 3 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b4537cd...9be02a6. Read the comment docs.

@typfel typfel merged commit b7a969f into develop Feb 6, 2018
@typfel typfel deleted the fix/format-strings-ZIOS-9204 branch February 6, 2018 09:06
zenkins added a commit that referenced this pull request Feb 6, 2018
Diff with previous:

	b7a969f Improve usage of format strings (#687)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
None yet
None yet

Successfully merging this pull request may close these issues.

None yet

3 participants