Add nullability annotations to all APIs. #88
Conversation
|
#87 is now merged in to master. let's pass CI and this is good to go. |
|
Awesome. You are my goto reviewer now then. |
nlutsenko
force-pushed
the
nlutsenko.nullability
branch
from
7604c93 to
78f1810
Compare
|
👍 |
|
|
| @@ -59,7 +60,7 @@ typedef void (^FBKVONotificationBlock)(id observer, id object, NSDictionary *cha | |||
| @param block The block to execute on notification. | |||
| @discussion On key-value change, the specified block is called. In order to avoid retain loops, the block must avoid referencing the KVO controller or an owner thereof. Observing an already observed object key path or nil results in no operation. | |||
| */ | |||
| - (void)observe:(id)object keyPath:(NSString *)keyPath options:(NSKeyValueObservingOptions)options block:(FBKVONotificationBlock)block; | |||
| - (void)observe:(nullable id)object keyPath:(NSString *)keyPath options:(NSKeyValueObservingOptions)options block:(FBKVONotificationBlock)block; | |||
There was a problem hiding this comment.
Why is 'object' nullable? Are we explicitly allowing no-ops here?
There was a problem hiding this comment.
Yup, inside implementation there is logic for bailing out early if the object is null.
There was a problem hiding this comment.
We can change it, but I think since we already are handling the null usecase, this is more permissive model. I don't feel strong about adding a constraint here, but we can defiently do it.
There was a problem hiding this comment.
I just wonder wether this is a useful for the API to expose or not. Again, the eternal debate of "should nullable/nonnull imply runtime semantics" or not...
My thinking is that observe is something that only logically makes sense on a nonnull value, but considering the existing API is out there and implicitly deals with nil, maybe it makes sense to not force null checks on clients. Not sold either way.
|
Passing back to you for discussion. |
|
Good discussion. Merge at will! |
Add nullability annotations to all APIs.
This adds nullability annotations to the entire API.
Everything is nonnull by default, unless otherwise specified.
Depends on #87 for latest Xcode in Travis.
cc @modocache