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

More type annotations #81

Merged
merged 8 commits into from
Mar 30, 2017

Conversation

erwald
Copy link
Member

@erwald erwald commented Mar 27, 2017

This pull request adds generics to RACMulticastConnection, RACBehaviorSubject and type annotations to RACSignal's +empty and +return methods.

It seems to me that it'd also make sense to add generics to RACChannelTerminal, RACChannel and RACSubject. Thoughts on that?

Erich Grunewald added 8 commits March 27, 2017 18:34
This means that `[RACSignal<NSString *> empty]` actually returns a
signal of NSString objects (even if it won't ever emit any values).
As an example, this allows us to call `[[signal publish] autoconnect]`
and retain the generic type of the original signal.
As an example, this allows us to call `[[signal publish] autoconnect]`
and retain the generic type of the original signal.
@erichoracek
Copy link
Contributor

This is great!

Aren't some of these classes private to the implementation to ReactiveObjC? E.g. RACEmptySignal and RACReturnSignal? What is the benefit of making them generic?

Re: RACSubject, as far as I can tell from our usage of the existing generics, it should be possible to make RACSubject generic to its value type since it is a direct subclass of RACSignal. This should work in the same way that NSMutableArray generic element type works with NSArray.

Re: RACChannel/RACChannelTerminal, that should work as well if you want to take a stab at it.

@erwald
Copy link
Member Author

erwald commented Mar 28, 2017

Yes, true, for RACEmptySignal and RACReturnSignal generics are not much use practically; I added it more on theoretical grounds. It makes sense to me semantically that a RACEmptySignal and RACReturnSignal has one associated type just like RACSignal does. But maybe it's just noise. Should I revert those changes?

Meanwhile, it seems the Travis CI build failed, although everything was hunky dory when running the tests locally. I'll have a look at that.

@erichoracek
Copy link
Contributor

That seems reasonable re: the internal types—it doesn't add any overhead to consumers.

@erwald
Copy link
Member Author

erwald commented Mar 29, 2017

All right, so I'll keep it as it is, then.

As for the failing Travis CI build, it seems to be a failure in three tests related to intervals. I don't see how that could possibly be caused by these type changes, and, moreover, the same tests pass locally.

Is it possible that the tests themselves are fickle, or that there are problems with the CI environment?

@erwald
Copy link
Member Author

erwald commented Mar 30, 2017

@erichoracek Possible to merge?

@erichoracek erichoracek merged commit 2754c16 into ReactiveCocoa:master Mar 30, 2017
erichoracek added a commit to erichoracek/ReactiveObjC that referenced this pull request Apr 7, 2017
erichoracek added a commit to erichoracek/ReactiveObjC that referenced this pull request Apr 7, 2017
Includes ReactiveCocoa#75, ReactiveCocoa#80, ReactiveCocoa#81, ReactiveCocoa#82, ReactiveCocoa#83, ReactiveCocoa#85, ReactiveCocoa#87, and ReactiveCocoa#88.

While this has no breaking changes in Obj-C, it will likely introduce breaking changes in Swift.
stuartofmine pushed a commit to stuartofmine/ReactiveObjC_2023 that referenced this pull request Oct 18, 2023
stuartofmine pushed a commit to stuartofmine/ReactiveObjC_2023 that referenced this pull request Oct 18, 2023
Includes ReactiveCocoa#75, ReactiveCocoa#80, ReactiveCocoa#81, ReactiveCocoa#82, ReactiveCocoa#83, ReactiveCocoa#85, ReactiveCocoa#87, and ReactiveCocoa#88.

While this has no breaking changes in Obj-C, it will likely introduce breaking changes in Swift.
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.

2 participants