Remove AppConstants - #24294
Conversation
0335064 to
7cc3758
Compare
| ) | ||
| zendeskSourcePlatform = bundle.infoValue(forKey: "WPZendeskSourcePlatform") | ||
| mobileAnnounceAppID = bundle.infoValue(forKey: "WPMobileAnnounceAppID") | ||
| authKeychainServiceName = bundle.infoValue(forKey: "WPAuthKeychainServiceName") |
There was a problem hiding this comment.
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.
All of these are just hard-coded values which don't have anything to do with build configurations
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 code switch brand { case ... here.
There was a problem hiding this comment.
two places where to place the values: Info.plist and the code switch brand { case ... here.
💯. 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 used switch 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.
|
I think I know why the builds are failing... Update: Finally got CI validation #24297 |
Apparently, Xcode/`xcodebuild` fails to parse it, see how https://buildkite.com/automattic/wordpress-ios/builds/26578/steps?jid=0195bb59-1f1a-40ad-89c9-3964c36cff28#0195bb59-1f1a-40ad-89c9-3964c36cff28/973-5502 failed and notice the build for this commit passes.
|
| App Name | WordPress Alpha |
|
| Configuration | Release-Alpha | |
| Build Number | pr24294-4b6cb2f | |
| Version | 25.8 | |
| Bundle ID | org.wordpress.alpha | |
| Commit | 4b6cb2f | |
| App Center Build | WPiOS - One-Offs #11818 |
|
| App Name | Jetpack Alpha |
|
| Configuration | Release-Alpha | |
| Build Number | pr24294-4b6cb2f | |
| Version | 25.8 | |
| Bundle ID | com.jetpack.alpha | |
| Commit | 4b6cb2f | |
| App Center Build | jetpack-installable-builds #10841 |
pmusolino
left a comment
There was a problem hiding this comment.
I just did a general smoke test, and I didn't find any crashes 👍
| <key>WPProductBlogURL</key> | ||
| <string>wordpress.org/news/</string> | ||
| <key>WPZendeskSourcePlatform</key> | ||
| <string>mobile_-_ios</string> |
There was a problem hiding this comment.
Should this be mobile_-_wp_ios? Is there a reason why we do not identify the app?
There was a problem hiding this comment.
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"

Self-explanatory.
To test:
Regression Notes
Potential unintended areas of impact
What I did to test those areas of impact (or what existing automated tests I relied on)
What automated tests I added (or what prevented me from doing so)
PR submission checklist:
RELEASE-NOTES.txtif necessary.Testing checklist: