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

Implemented RefCount Operator #407

Merged
merged 11 commits into from
Oct 9, 2013
Merged

Conversation

johnhmarks
Copy link
Contributor

Please note that I placed the unit tests under the test root rather than inline with the implementation as has been done for other operators. This is due to a bug in IDEA that prohibits running unit tests in folders designated as source rather than test. I can see that a bunch of other operators follow this convention so hopefully that's okay.

@cloudbees-pull-request-builder

RxJava-pull-requests #310 ABORTED

@johnhmarks
Copy link
Contributor Author

I notice this was aborted due to a timeout in the Scala language adaptors tests. I didn't make any changes in this area, so am guessing there's an intermittent bug somewhere.

@cloudbees-pull-request-builder

RxJava-pull-requests #311 ABORTED

@samuelgruetter
Copy link
Contributor

The problem with the Scala tests should be fixed by #408, but that only saves 48 seconds. There must be also another problem with the cloudbees-pull-request-builder...

@benjchristensen
Copy link
Member

Hopefully the builds should improve now, I've changed the CloudBees config to allow twice as much time.

@benjchristensen
Copy link
Member

Thank you for implementing refCount, it's been one I've wanted for a while but haven't had time to do.

Will review it and merge as soon as I can.

* @return a {@link Observable}
* @param that a {@link ConnectableObservable}
*/
public static <T> Observable<T> refCount(ConnectableObservable<T> that) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be a no arg instance method?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi George, thanks for taking the time to review the code. Indeed there is a no arg instance method. I also included the static version to follow the pattern using in Observable.java where many operators have static and instance variants that delegate to the statics. It has an added bonus that it means you can test the operator on a mocked instance. I'm happy to remove the static method and test against a concrete instance though. Cheers, John

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, on closer inspection, those operations on Observable that have both static and instance variants like concat, merge and zip could be special cases. I'll remove the static method and rejig the tests.

@cloudbees-pull-request-builder

RxJava-pull-requests #314 ABORTED

benjchristensen added a commit that referenced this pull request Oct 9, 2013
@benjchristensen benjchristensen merged commit 490ef86 into ReactiveX:master Oct 9, 2013
@benjchristensen
Copy link
Member

Thanks @johnhmarks for submitting this.

This was referenced Oct 23, 2013
rickbw pushed a commit to rickbw/RxJava that referenced this pull request Jan 9, 2014
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.

5 participants