Skip to content
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

Make WordPressOrgXMLRPCApiError conform to CustomNSError #790

Open
wants to merge 7 commits into
base: trunk
Choose a base branch
from

Conversation

mokagio
Copy link
Contributor

@mokagio mokagio commented Apr 15, 2024

See discussion at #782 (comment)


  • Please check here if your pull request includes additional test coverage.
  • I have considered if this change warrants release notes and have added them to the appropriate section in the CHANGELOG.md if necessary.

Base automatically changed from mokagio/coreapi-package to trunk April 16, 2024 16:14
domain: WordPressOrgXMLRPCApiErrorDomain,
code: code,
userInfo: userInfo
)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you create a WordPressOrgXMLRPCApiError error here, cast it to NSError, and return it to Objective-C code, the instance can be cast back to WordPressOrgXMLRPCApiError in Swift code when the error is passed from Objective-C code to Swift.

I don't suppose you can cast this NSError instance to WordPressOrgXMLRPCApiError? It would be pretty magical if you can. 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, this is a shortsighted leftover from before I realized that the error domain constant could be defined in APIInterface and therefore be available to packages in both Objective-C and Swift.

I shall remove this utils like I did for WordPressComRestApiErrorDomain.

This ensures that the error domain is the same between SPM and
Framework/CocoaPods builds.
Jumping through this hoop was necessary to avoid the Swift compiler
automatically generating an error domain for the `Error` and making it
impossible to successfully redefine the domain at the `CustomNSError`
conformance site.
@mokagio mokagio force-pushed the mokagio/coreapi-customnserror-xmlrpc branch from 22dcdd8 to ac15e9a Compare April 17, 2024 06:50
Comment on lines +3 to +18
public struct WordPressOrgXMLRPCApiError: Error {

/// Error constants for the WordPress XML-RPC API
@objc public enum Code: Int, CaseIterable {
/// An error HTTP status code was returned.
case httpErrorStatusCode
/// The serialization of the request failed.
case requestSerializationFailed
/// The serialization of the response failed.
case responseSerializationFailed
/// An unknown error occurred.
case unknown
}

let code: Code
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@crazytonyli I ended up having to wrap the error code enum into an Error type like you did for the REST API error.

Otherwise, between the @objc visibility (needed because WordPress-Authenticator uses it 🙄 ) and the Error conformance, I couldn't find a way to stop the compiler from automatically generating WordPressOrgXMLRPCApiErrorDomain for @objc public enum WordPressOrgXMLRPCApiError: Int, Error.

I think this is an okay solution. Apart for the fact that's the only one I could get to work, it's, as far as I can see, the same setup as the other error, which makes the implementation consistent.

@@ -128,15 +119,10 @@ class WordPressOrgXMLRPCApiTests: XCTestCase {
failure: { (error, _) in
expect.fulfill()

XCTAssertTrue(error is WordPressOrgXMLRPCApiError)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the only change in behavior from the rewrite.

The NSError-converted error (via asNSerror()) that the API passes to this failure branch doesn't convert back to WordPressOrgXMLRPCApiError.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I might have missed something. Does that mean the error in the failure block was WordPressOrgXMLRPCApiError, but in this PR it's changed to something else?

@mokagio mokagio marked this pull request as ready for review April 17, 2024 07:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants