-
Notifications
You must be signed in to change notification settings - Fork 16
Change Swift package to provide source code #846
Conversation
Generated by 🚫 Danger |
| public var avatarURL: String? | ||
| public var dateCreated: Date? | ||
| public var emailVerified: Bool = false | ||
| public var linkedUserID: NSNumber? |
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 original properties in the original Objective-C code are null_unspecified or implicitly unwrapped optional. I can keep the original code and make the new properties IUO, too. But that does not feel right.
The new code now uses optional, which means the app will need to update their codebase to deal with the new optional properties instead of IUO properties.
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 need to translate this function to Swift, because it's used in a Swift module. I'll explain a little bit more of why this is needed in the upcoming app PR (which would provide a full context).
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.
This looks good. We'll just need to make sure we test the features that needed rewrite.
| extension ServiceRemoteWordPressComREST { | ||
| public var wordPressComRestApi: WordPressComRestApi { | ||
| self.wordPressComRESTAPI as! WordPressComRestApi | ||
| } |
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.
We have to use an interface type in the ServiceRemoteWordPressComREST.h, because the implementation is in the Swift WordPressKit module.
In practice, WordPressComRestApi is the only acceptable instance to create ServiceRemoteWordPressComREST, so, we'll do a little bit hack here to force cast in runtime.
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.
Same as the WordPressOrgXMLRPCApi property below.
kean
left a comment
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.
This is great! And it's a straightforward changes with minimum code conversions.
|
As mentioned in the PR description, I'll close this PR as the changes are being integrated into the app repo. |
Description
There are some difficulties in using a binary package. This PR refactors the source code a little bit so that we can provide source code instead of a pre-built binary.
I have split the changes into individual self-contained commits. Most of the diff comes from adding
importstatements. The actual code changes are contained in their own commit, which only includes a small number of changes.Next steps
We don't need to merge this PR. Once this PR passes code review, I'll copy the source code over to the WordPress-iOS repository. And, the app will stop using this library at that point.
After that, I'll copy the test code over to the WordPress-iOS project and set up a new test target there, so that the existing WordPressKit unit tests continue to run as part of the app CI pipeline.
Testing Details
ℹ Please replace this with a clear and concise description of the steps required to validate this pull request.
CHANGELOG.mdif necessary.