-
Notifications
You must be signed in to change notification settings - Fork 16
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 PostServiceRemoteExtended
and implementations
#784
base: trunk
Are you sure you want to change the base?
Conversation
They will be moved into WordPress and Jetpack. The reason for this is that they are only used in WordPress and Jetpack and that they are Swift extensions of Objective-C types which we are trying to isolate. It's simpler to move these leaf types into the only consumer that uses them than trying to re-architect them.
87e9115
to
58a6386
Compare
public init(parameters: RemotePostCreateParameters) { | ||
self.parameters = parameters | ||
} | ||
|
||
public func encode(to encoder: Encoder) throws { |
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.
When the type became public
, a public init
became necessary because the compiler does not synthesize one outside of the package.
As for encode(to:)
, it needs to be public
because Encodable
is declared as part of the public
type interface.
🤔 In hindsight, I suppose the Encodable
part could be moved into an internal
extension, but I think it's okay to leave it as is. Let me know if you can think of a strong reason to hide it.
case unparsableResponse(response: HTTPURLResponse?, body: Data?, underlyingError: Error) | ||
case unparsableResponse(response: HTTPURLResponse?, body: Data?, underlyingError: Error = URLError(.cannotParseResponse)) | ||
/// Other error occured. | ||
case unknown(underlyingError: Error) | ||
|
||
static func unparsableResponse(response: HTTPURLResponse?, body: Data?) -> Self { | ||
return WordPressAPIError<EndpointError>.unparsableResponse(response: response, body: body, underlyingError: URLError(.cannotParseResponse)) | ||
} |
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 didn't look up when, but relatively recently, I think, Swift acquired default values for associated parameters. So we can remove this builder method in favor of defining the underlying error as a default for the case.
This is just a convenience refactor to make future diff clearer
As per Swift API naming guidelines on arguments with default values
@objc func setInvalidTokenHandler(_ handler: @escaping () -> Void) { | ||
invalidTokenHandler = handler | ||
} |
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.
Became redundant once invalidTokenHandler
became a property in the interface.
@mokagio This code is a work in progress, and the respective PRs that integrate it in WordPress-iOS haven't been merged yet. Please hold off on making changes to this code until 24.8. Initially, the idea and the naming of |
Hi @kean 🤔 There I was thinking hacking around WordPressKit was safe because it was relatively quiet these days 😅 What's your timeline for that work? My priority right now is to get WordPressKit split into packages so that the majority of it can then go into WordPress and we can avoid back and forth like this. Maybe I can help get your project over the line?
Interesting. Did you consider making all the addition in WordPress itself? Or perhaps there's internal utils that need to be accessed in WordPressKit to get what you need done? "Replaced" is an interesting choice of word. Do you mean that the now there are methods that are no longer in use? Or is it a matter of the new feature you and team are working on needs a lot of new methods that will live in parallel to the existing ones? Thanks for the info. |
We've added new methods for creating and updating posts. I'm not sure it's possible to implement them outside of the framework, but I haven't tried or considered it. The old methods will no longer be used by the app once 24.8 rolls out. |
They will be moved into WordPress and Jetpack.
The reason for this is that they are only used in WordPress and Jetpack and that they are Swift extensions of Objective-C types which we are trying to isolate.
It's simpler to move these leaf types into the only consumer that uses them than trying to re-architect them.
Description
Fixes #
ℹ Please replace the above with a link to the issue this pull request addresses, as well as a summary of the implementation details.
Testing Details
ℹ Please replace this with a clear and concise description of the steps required to validate this pull request.
CHANGELOG.md
if necessary.