-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Remove AppConstants #24294
Remove AppConstants #24294
Changes from all commits
feab212
91cb125
06bfd2d
b783aab
357a646
b4505e2
6197f93
53a8606
5717c4c
cf3fcf3
292dbac
f2619f6
1ece6e8
1ba6b4f
7cc3758
6054c9e
8bbd61c
b87f900
4b6cb2f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -20,8 +20,20 @@ | |
<string>${WP_BUILD_CONFIGURATION}</string> | ||
<key>WPAppURLScheme</key> | ||
<string>${WP_APP_URL_SCHEME}</string> | ||
<key>WPJetpackAppURLScheme</key> | ||
<string>${WP_JETPACK_APP_URL_SCHEME}</string> | ||
<key>WPJetpackAppURLScheme</key> | ||
<string>${WP_JETPACK_APP_URL_SCHEME}</string> | ||
<key>WPProductTwitterHandle</key> | ||
<string>@WordPressiOS</string> | ||
<key>WPProductTwitterURL</key> | ||
<string>twitter.com/WordPressiOS</string> | ||
<key>WPProductBlogURL</key> | ||
<string>wordpress.org/news/</string> | ||
<key>WPZendeskSourcePlatform</key> | ||
<string>mobile_-_ios</string> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should this be There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I copied the value from https://github.com/wordpress-mobile/WordPress-iOS/blob/trunk/WordPress/Classes/Utility/App%20Configuration/AppConstants.swift: static let zendeskSourcePlatform = "mobile_-_ios" |
||
<key>WPMobileAnnounceAppID</key> | ||
<string>2</string> | ||
<key>WPAuthKeychainServiceName</key> | ||
<string>public-api.wordpress.com</string> | ||
<key>BGTaskSchedulerPermittedIdentifiers</key> | ||
<array> | ||
<string>org.wordpress.bgtask.weeklyroundup</string> | ||
|
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.
All of these are just hard-coded values which don't have anything to do with build configurations. Maybe we can just have some readonly variables or something? I don't think all these values need to be in the Info.plist files.
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.
True. However, they do change from WordPress and Jetpack. Maybe we could derive them from
brand
/WPAppBrand
. But then we'd have sort of two places where to place the values:Info.plist
and the codeswitch brand { case ...
here.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.
💯. The idea is to specify configuration in data:
Info.plist
and.xcconfig
. It's not always feasible, but you generally shouldn't have to search the codebase to find these values – it should be in configuration files, in data. I usedswitch brand
sparingly.There are multiple advantages to storing these values in data and not code: easier to find, can be modified without recompiling the app, easier to edit with scripts if needed.