Tweak a few file to allow WordPressData to compile - #24375
Conversation
This way, when `PublicizeService` moves to WordPressData, it won't need the dependency used in that code.
f39cba1 to
6bc1aed
Compare
| /// | ||
| public func migrateAuthKey(for username: String) { | ||
| guard AppConfiguration.isJetpack, | ||
| guard BuildSettings.current.brand == .jetpack, |
There was a problem hiding this comment.
Using BuildSettings over AppConfiguration allows moving to WordPressData where the latter is not available.
| { | ||
| if (self.account) { | ||
| return self.account.wordPressComRestApi; | ||
| return self.account._private_wordPressComRestApi; |
There was a problem hiding this comment.
wordPressComRestApi uses WordPressAuthenticationManager which depends on WordPressAuthenticator which we don't want to add to WordPressData.
There was a problem hiding this comment.
Does this method need to be in WordPressData? Would it make sense to keep inside the app targets like the similar WPAccount extension?
There was a problem hiding this comment.
Good question... I'll have to check.
There was a problem hiding this comment.
After a quick check, I think it should be possible to move it to 1) Swift and 2) the apps targets. This will also require moving some additional logic for logging / Zendesk / customer support, but it should not be too complex...
There was a problem hiding this comment.
Addressed via 75f416c (#24375) @kean — thanks again for the suggestion
| /// Returns the local image for the icon representing the social network. | ||
| var localIconImage: UIImage { | ||
| WPStyleGuide.socialIcon(for: rawValue as NSString) | ||
| } | ||
|
|
There was a problem hiding this comment.
socialIcon(for service: NSString) -> UIImage is defined in the apps target / Keystone, which WordPressData does not have access to.
By moving it away from here, we unblock adding this required extension to the framework.
|
| App Name | WordPress | |
| Configuration | Release-Alpha | |
| Build Number | 26995 | |
| Version | PR #24375 | |
| Bundle ID | org.wordpress.alpha | |
| Commit | 75f416c | |
| Installation URL | 0ri5om2m3uv30 |
|
| App Name | Jetpack | |
| Configuration | Release-Alpha | |
| Build Number | 26995 | |
| Version | PR #24375 | |
| Bundle ID | com.jetpack.alpha | |
| Commit | 75f416c | |
| Installation URL | 0tvuiusqnh4tg |
Apologies for the annoying diff due to the change from methods to computed vars. I suppose I could have left the implementations as method, but it felt cleaner to move them to more Swift-idiomatic computed vars.
bea1e47 to
75f416c
Compare
|
The tests failed in CI before 
|
kean
left a comment
kean
left a comment
There was a problem hiding this comment.
LGTM. If you need something rewriting from Objective-C, let me know.
4 participants
Discovered these while working on #24378.
Part of #24165.