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

Fix ConnectableObservable: scheduler is not forwarded #492

Merged
merged 2 commits into from Feb 24, 2020

Conversation

jcafhe
Copy link
Collaborator

@jcafhe jcafhe commented Feb 3, 2020

This PR fixes ConnectableObservable.connect(self, scheduler) method, where the source observable is not subscribed with the downstream subscribe scheduler.

I guess this is related to the fact that scheduler is a keyword argument only, so scheduler must be given as subscribe(observer, scheduler=scheduler), otherwise it will be dismissed.

A simple test case has been added.

This should fix some operators which are based on connect(scheduler) method like ref_count, and, by extension, to share. This should also fix #489.

@jcafhe jcafhe changed the title Fix scheduler connectable observable Fix ConnectableObservable: scheduler is not forwarded Feb 3, 2020
@frederikaalund
Copy link
Contributor

Fixes #489

@MainRo
Copy link
Collaborator

MainRo commented Feb 3, 2020

I guess this is related to the fact that scheduler is a keyword argument only, so scheduler must be given as subscribe(observer, scheduler=scheduler), otherwise it will be dismissed.

Yes, the second argument to the subscribe method is on_error. So scheduler has to be set with kw unless the 3 callbacks are provided before.

Nice!

@jcafhe jcafhe force-pushed the fix-scheduler-ConnectableObservable branch from a878822 to ee14b7c Compare February 24, 2020 13:51
@coveralls
Copy link

Coverage Status

Coverage remained the same at 92.924% when pulling ee14b7c on jcafhe:fix-scheduler-ConnectableObservable into 5fc2cfb on ReactiveX:master.

@jcafhe jcafhe merged commit cc3588d into ReactiveX:master Feb 24, 2020
@jcafhe jcafhe deleted the fix-scheduler-ConnectableObservable branch February 24, 2020 14:28
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.

Scheduler provided in subscribe is not used for all observable factories
4 participants