-
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
SPM Prep – Use WordPressComRESTAPIInterfacing
in DomainServiceRemote
and AccountServiceRemoteREST
#766
Conversation
@@ -23,7 +23,7 @@ extension AccountServiceRemoteREST { | |||
oAuthClientID: String, | |||
oAuthClientSecret: String, | |||
success: @escaping (() -> Void), | |||
failure: @escaping ((NSError) -> Void)) { | |||
failure: @escaping ((Error) -> Void)) { |
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 don't think this counts as a breaking change, does it?
The API became looser not stricter. Any code passing (NSError) -> Void
to failure should still compile now that the expected type is (Error) -> Void
.
Anyways, if it's a breaking change, I'll just add it to the existing list 😅
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.
IMO, it's still a breaking change. The consumers may use NSError.domain
in their code, which won't compile with this updated code.
One way to avoid this breaking change is casing as NSError in the failure
callback below: failure(error as NSError)
.
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.
If that's okay with you, I'd keep Error
and count this as a breaking change, then.
Error
is more Swifty, which I think is the best approach going forward.
I'll merge under that assumption and the approval you already gave. Happy to followup with a fix PR if needed.
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 don't have a strong opinion on this.
/// - Note: `parameters` has `id` instead of the more common `NSObject *` as its value type so it will convert to `AnyObject` in Swift. | ||
/// In Swift, it's simpler to work with `AnyObject` than with `NSObject`. For example `"abc" as AnyObject` over `"abc" as NSObject`. | ||
- (NSProgress * _Nullable)get:(NSString * _Nonnull)URLString | ||
parameters:(NSDictionary<NSString *, NSObject *> * _Nullable)parameters | ||
parameters:(NSDictionary<NSString *, id> * _Nullable)parameters |
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.
Does the explanation make sense?
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.
Yep. It makes sense to me. Some Foundation API uses the same "fake generic" signature, i.e. UserDefaults
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.
Awesome! TIL
…EST` Instead of `NSError`. This way, we'll be able to pass the error to the `WordPressComRESTAPIInterfacing` version without needing to make any conversion. As far as I could see, the only consumer already worked in terms of `Error`. https://github.com/wordpress-mobile/WordPressAuthenticator-iOS/blob/30aada20beb9e0871f4da35a5634501536898b3b/WordPressAuthenticator/Services/WordPressComAccountService.swift#L31-L44
4776cd8
to
52a7ea4
Compare
} | ||
|
||
public func post( | ||
_ URLString: String, | ||
parameters: [String: NSObject]?, | ||
parameters: [String: Any]?, |
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 don't think this is a breaking change, because [String: NSObject]
is a "sub-type" of [String: Any]
, thus can be passed in here?
Description
A small PR to get feedback on my approach to adopt the
WordPressComRESTAPIInterfacing
abstraction layer from #760 in Swift.This required a few minor tweaks, mainly to make sure the code across Objective-C and Swift deals with
Any
instead ofNSObject
. I think this is okay.Any
is as loose a type as it comes in Swift, but the loss of type safety is only a matter of making the two languages working together. All the code I've dealt with so far uses stricter types at runtime, of course.Testing Details
Ensure CI is green.
Next Steps
Assuming the implementation direction I've taken is appropriate, after this lands I'll move on with porting it to the rest of the Swift files.
CHANGELOG.md
if necessary. — N.A.