Refactor WordPressComRestApiError #696
Conversation
…iError This is an unfortunate comprosie we need to make during a refactor(*). After the refactor is done, the `error` variable here should be of type `WordPressAPIError<WordPressComRestApiEndpointError>`. That means, at the end, we can access the 'error code' in a more strongly-typed way. (*): A WordPressComRestApi refactor to make it return `WordPressAPIError` in its Swift API, instead of `NSError`.
| switch self { | ||
| case let .endpointError(endpointError): | ||
| return (endpointError as NSError).code | ||
| // Use negative values for other cases to reduce chances of collision with `EndpointError`. |
| /// In theory, we should not need to implement this bridging, because we should never cast errors to `NSError` | ||
| /// instances in any error handling code. But since there are still Objective-C callers, providing this custom bridging | ||
| /// implementation comes in handy for those cases. See the `WordPressComRestApiEndpointError` extension below. | ||
| extension WordPressAPIError: CustomNSError { |
mokagio
dismissed
their stale review
I approved out of habit, when what I wanted to do was "this commit looks good, let's move to the next"
| let errorWithLocalizedMessage = NSError(domain: nsError.domain, code: nsError.code, userInfo: userInfo) | ||
| return errorWithLocalizedMessage | ||
|
|
||
| let message = NSLocalizedString("Limit reached. You can try again in 1 minute. Trying again before that will only increase the time you have to wait before the ban is lifted. If you think this is in error, contact support.", comment: "Message to show when a request for a WP.com API endpoint is throttled") |
There was a problem hiding this comment.
Good occasion to add key as well? I mean, I understand this is a refactor so behavior should not change, but I think we can squeeze this in:
| let message = NSLocalizedString("Limit reached. You can try again in 1 minute. Trying again before that will only increase the time you have to wait before the ban is lifted. If you think this is in error, contact support.", comment: "Message to show when a request for a WP.com API endpoint is throttled") | |
| let message = NSLocalizedString( | |
| "wordpresskit.api.message.endpoint_throttled", | |
| value: "Limit reached. You can try again in 1 minute. Trying again before that will only increase the time you have to wait before the ban is lifted. If you think this is in error, contact support.", | |
| comment: "Message to show when a request for a WP.com API endpoint is throttled" | |
| ) |
There was a problem hiding this comment.
Code squeezed. 3cc7a80
| let nserror = self.processError(response: response, originalError: error).flatMap { $0 as NSError } | ||
| failure(nserror ?? (error as NSError), response.response) |
There was a problem hiding this comment.
The original naming of the value made me feel confused when reading (error as NSError).
What do you think of processedError
| let nserror = self.processError(response: response, originalError: error).flatMap { $0 as NSError } | |
| failure(nserror ?? (error as NSError), response.response) | |
| let processedError = self.processError(response: response, originalError: error).flatMap { $0 as NSError } | |
| failure(processedError ?? (error as NSError), response.response) |
|
It's a breaking change for WordPressAuthenticator. I suggest adding a deprecation for backward compatibility: I can do that in the scope of #704 |
Yes indeed. As noted in the changelog file, this PR introduces a breaking change.
The new typealias still introduces a breaking change, because BTW, I have made some changes (wordpress-mobile/WordPress-iOS#22434, woocommerce/woocommerce-ios#11755, and wordpress-mobile/WordPressAuthenticator-iOS#829) to adopt these breaking changes. They are still in draft, because we haven't released a new WordPressKit version yet. But, if these breaking changes are blocking you, I can look into releasing new versions of the libraries? |
Description
In upcoming PRs, I'll add functions to
WordPressComRestApiwhich useURLSessionto make API calls. The newly addedURLSessionreturnsenum WordPressAPIErrortype. That means,NSErrorinstances that are returned byWordPressComRestApiwill change internally: In cases where returns aWordPressComRestApiError, they will returnWordPressAPIErrorafter the refactor(*). That means the domain and code value in thoseNSErrorinstances will be changed too. This change of course impacts the error handling code on the app side.Note
There are some info in
WordPressKit/WordPressAPIError+NSErrorBrdige.swiftthat I don't repeat here. Please have a read there too.There are mainly two kinds of code that handles
NSErrorreturned byWordPressComRestApi:domainandcode:domain == "WordPressComRestApiError" && code == WordPressComRestApiError...WordPressComRestApiError:error as? WordPressComRestApiError.Once the aforementioned refactor is implemented, it's pretty easy to migrate the second error handling code: by simply resolving Swift compiler errors.
However, the first error handling code is more tricky to resolve. Because it's largely used in Objective-C code which can't access
enum WordPressAPIError<EndpointError>Swift type.One way to solve the issue is making sure returned
NSErrorinstances (their domain, code, user info) do not change, even though the type of errors from where they are casted has been changed: the threeNSErrorinstances below are equal.WordPressComRestApiError.<code> as NSError(old code)WordPressComRestApiEndpointError(code: <code>) as NSError(new code).WordPressComRestApiEndpointError(code: <code>) as NSError(new code).(*): Since we haven't implemented the new API call functions, this PR changes the existing functions to return
WordPressComRestApiEndpointErrorinstead ofWordPressAPIErrorfor now. But eventually, the errors will beWordPressAPIError<WordPressComRestApiEndpointError>Note
It might be easier to review this PR commit by commit.
Testing Details
See the updated unit tests.
CHANGELOG.mdif necessary.