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

Better nullability and error handling #315

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

ZevEisenberg
Copy link

@ZevEisenberg ZevEisenberg commented Sep 18, 2019

Checklist

  • I've checked that all new and existing tests pass
  • I've updated the documentation if necessary
  • I've added an entry in the CHANGELOG to credit myself

Description

Fixes #290

Also does more idiomatic error handling:

Wrong:

NSError *error;
NSArray *result = [someObject someMethod error:&error];
if (!error) {
    // assume success
}

Correct:

NSError *error;
NSArray *result = [someObject someMethod error:&error];
if (result) {
    // assume success
}

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 like for 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.

Copy link
Owner

@AliSoftware AliSoftware left a 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;
Copy link
Owner

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?

Copy link
Owner

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 😅)

Copy link
Author

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.

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.

Static Analysis results ran from XCode 9.4.1
2 participants