-
Notifications
You must be signed in to change notification settings - Fork 212
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
Bind observable to collection of observers #166
Conversation
…a collection of obsevers
dbfa771
to
7dfc999
Compare
@sgtsquiggs This is a nice feature. I really like and already have something exactly like this in my codebase. It would be nice to add |
@bobgodwinx There was discussion in #163 where I also agreed it was a pretty great addition. For |
I guess if you're manipulating UI controls (as in the example in #163) you may already be using Drivers to do so. |
In that case just |
@freak4pc it's useful when you have more than one UI that changes on an event. So you just get the updates and |
I think that if we're overloading |
Sounds like a deal let's go with |
Cool. So, @sgtsquiggs would you like to take care of that? Also, please look at the contribution guidelines: https://github.com/RxSwiftCommunity/RxSwiftExt/blob/master/CONTRIBUTING.md#submitting-a-pull-request You need to add a playground page and update the README documenting these operators. |
Hmm I can't seem to get the playground working. I'm not too familiar with Carthage, so may be that. It seems to be loading latest released RxSwiftExt from this repo instead of my local modified copy - so it does not have my new operator available. I had the same issue with adding tests; my test is the only one that is using Any pointers? |
@sgtsquiggs If you want to put your work on this branch, I can try and take a look |
@sgtsquiggs You forgot to make your extensions |
I seem to have broken the Playground with this last commit. It hangs before executing any code. |
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.
Two questions. Also, it doesn't hang for me.
Readme.md
Outdated
@@ -556,6 +557,28 @@ next((Test Class, 13)) | |||
completed | |||
``` | |||
|
|||
#### bindCollection |
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 should be bindMany like the other places.
|
||
let values = ["one", "two", "three", "four"] | ||
|
||
let observers = [scheduler.createObserver(String.self), |
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.
Can you make a test that confirms that All disposables are disposed ?
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.
Can do. I did not test because of CompositeDisposable's existing tests I wasn't aware CompositeDisposable was RxSwift and not RxSwiftExt 😑 .
Does this PR need any additional work? |
@sgtsquiggs Doesn't seem so. I want to give the discussion in ReactiveX/RxSwift#1702 a fair opportunity. Let's give it two more weeks or so, things move a bit slower there :) |
In light of ReactiveX/RxSwift#1702 (comment) I think it would be best to use |
@sgtsquiggs There has been good movement on ReactiveX/RxSwift#1702 . Kruno suggests adding this only in RxSwift 5, but im not 100% sure since it doesn't introduce breakage. But he's open to adding it, so I suggest we keep following on Core. |
This is coming in RxSwift 5. Closing. ReactiveX/RxSwift#1702 |
An extension on ObservableType to allow
.bind(to:)
with a collection of observers, and an extension on Collection (where Iterator.Element == Disposable) to allow.disposed(by:)
with a collection of Disposables.Fixes #163