-
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
Move WPAccount
wordPressComRestAPI
definition to Swift
#24230
Conversation
|
App Name | ![]() |
|
Configuration | Release-Alpha | |
Build Number | pr24230-50f3d06 | |
Version | 25.8 | |
Bundle ID | org.wordpress.alpha | |
Commit | 50f3d06 | |
App Center Build | WPiOS - One-Offs #11725 |
|
App Name | ![]() |
|
Configuration | Release-Alpha | |
Build Number | pr24230-50f3d06 | |
Version | 25.8 | |
Bundle ID | com.jetpack.alpha | |
Commit | 50f3d06 | |
App Center Build | jetpack-installable-builds #10753 |
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.
I reviewed the new Swift implementation side-by-side with the old one, and tested the login/logout in the app. It seems like a simple and solid change. I left two small (nit) comments.
get { | ||
if let api = objc_getAssociatedObject(self, &apiAssociatedKey) as? WordPressComRestApi { | ||
return api | ||
} else { |
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.
(nit) I'd suggest removing the else
to reduce the indenting a bit.
if let api = objc_getAssociatedObject(self, &apiAssociatedKey) as? WordPressComRestApi { | ||
return api | ||
} else { | ||
if authToken.isEmpty { |
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.
(nit) seems like a good place for a guard
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.
Thanks for both guard
suggestions!
Finally got to address them in 50f3d06
1755968
to
50f3d06
Compare
Part of #24165.
Following up on a suggestion by @kean internally, ref p1741953158905339-slack-C08HQR4C2TS, this PR splits the definition of the
wordPressComRestAPI
property inWPAccount
from its implementation between the Objective-C and Swift layer in an attempt to allow movingWPAccount
away from the app target(s) and into WordPressData.The unit tests are green, which is encouraging.
I also tested this on device via Jetpack.