-
Notifications
You must be signed in to change notification settings - Fork 602
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
Better nullability and error handling #315
base: master
Are you sure you want to change the base?
Conversation
… is an issue reading the input file.
…ndefined unless result signals failure via a nil value.
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.
Nice!
@@ -43,7 +43,7 @@ @interface OHHTTPStubsProtocol : NSURLProtocol @end | |||
|
|||
@interface OHHTTPStubs() | |||
+ (instancetype)sharedInstance; | |||
@property(atomic, copy) NSMutableArray* stubDescriptors; | |||
@property(atomic, readonly) NSMutableArray* stubDescriptors; |
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.
Problem with this is, it's a reference type. So that means that users can't set stubDescriptors = newValue
but they can [stubDescriptors append:…]
though can't they?
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.
Oh wait it's in the private category so not exposed to the consumer is it? (Not easy to review from phone 😅)
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.
(Forgot to reply originally) copy
never works on properties of NSMutableCopying
type, because the setter calls -copy
, which returns an immutable copy, i.e. an NSArray
in this case. So you’d set a mutable array, and the value that gets stored would be immutable. Fun edge case in Obj-C land that should probably be a compile error.
Checklist
Description
Fixes #290
Also does more idiomatic error handling:
Wrong:
Correct:
Motivation and Context
The main motivation was addressing static analysis warnings I encountered when analyzing an Objective-C project. The error handling was just something I fixed along the way.
Note: I also added a couple of missing generics annotations which may affect the public interface. If they are imported into Swift, they will go from
[Any]
to[OHHTTPStubsDescriptor]
.This is a breaking change for anyone who is using the output of this method in Swift.Actually, it may not be breaking, since in Swift, they were probably already doing something likefor descriptor in descriptors as! [OHHTTPStubsDescriptor]
, and after this update, that line will throw a compile warning, not an error. So this can probably be a bugfix, not a major release.Testing: I added a couple of missing tests for error throwing cases, and made sure all the tests were passing.