-
Notifications
You must be signed in to change notification settings - Fork 496
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
Add nullability annotations to RACSubscriber #18
Conversation
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.
This is great! Just one minor change I think we should make.
@@ -46,6 +46,6 @@ | |||
/// A subscriber may receive multiple disposables if it gets subscribed to | |||
/// multiple signals; however, any error or completed events must terminate _all_ | |||
/// subscriptions. | |||
- (void)didSubscribeWithDisposable:(RACCompoundDisposable *)disposable; | |||
- (void)didSubscribeWithDisposable:(RACCompoundDisposable * _Nonnull)disposable; |
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.
Could we use NS_ASSUME_NONNULL_BEGIN
and NS_ASSUME_NONNULL_END
around the protocol instead of explicitly specifying _Nonnull
here?
👍 @mdiep I just amended my commit to use |
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.
Shouldn't it be sendNext:(nullable id)value
and sendError:(nullable NSError *)error;
? As far as I can tell from the docs, these can both be nil
.
@erichoracek is correct. |
Yep, good catch @erichoracek. Too much rushing on my part. Updated. |
@@ -10,6 +10,8 @@ | |||
|
|||
@class RACCompoundDisposable; | |||
|
|||
NS_ASSUME_NONNULL_BEGIN; |
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.
Just a quick nit: the NS_ASSUME_NONNULL_*
family of macros doesn't need a trailing semicolon in my experience.
@@ -49,3 +51,5 @@ | |||
- (void)didSubscribeWithDisposable:(RACCompoundDisposable *)disposable; | |||
|
|||
@end | |||
|
|||
NS_ASSUME_NONNULL_END; |
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.
Same comment re: semicolon usage applies here.
Thanks! |
…ove-guards extobjc: Remove header guards.
Add nullability annotations to RACSubscriber
In general, it seems helpful to have nullability annotations specified within
RACSubscriber.h
, though perhaps there's a strong reason for which this file hasn't already been annotated that I'm not aware of.However, according to SE-0140:
With SE-0140, which has already been implemented in Swift 3.0.1 (available in the Xcode 8.1 beta 3), it is important that nullability annotations be specified on
RACSubscriber
. Otherwise, whenRACSubscriber.sendNext()
is passed anOptional.none
from within Swift, Swift will (post Swift 3.0.1) be bridge this toNSNull
, when previously (pre-Swift 3.0.1) it was visible within the Objective-CsendNext:
implementation simply asnil
. This results in a change in behavior when ReactiveObjC is compiled with the Swift 3.0.1 compiler@raylillywhite and I observed this change in behavior in Xcode 8.1 beta 3, and have verified that adding nullability annotations restores the previous behavior that users of ReactiveObjC are relying on.
Here's the difference in the Swift generated interface before and after this change:
Previous Swift generated interface (from Xcode 8.1 beta 3 with Swift 3.0.1)
New Swift generated interface (from Xcode 8.1 beta 3 with Swift 3.0.1):
Relatedly, there seems to be a bug or limitation in the Swift compiler that causes
ReactiveObjCBridging
'sRACSignal.toRACSignal()
function to bridgeOptional.none
toNSNull
(in next events' values) even after added the nullability annotation to-[RACSubscriber sendNext:]
. @raylillywhite and I are in the process of investigating this and have created ReactiveCocoa/ReactiveObjCBridge/pull/5 to address this.