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.

Sign up for GitHub

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

Jump to bottom

1.x: Implements BlockingSingle #3286

Merged
merged 1 commit into from
Nov 11, 2015
Merged

1.x: Implements BlockingSingle #3286

merged 1 commit into from
Nov 11, 2015

Conversation

hyleung
Copy link

@hyleung hyleung commented Sep 2, 2015

Adds BlockingSingle (issue #3252), the blocking version of rx.Single.

BlockingSingle has the following methods:

  • from(Single) -- factory method for creating a BlockingSingle from a
    Single
  • value() -- returns the value emitted from the Single
  • toFuture() -- returns a java.util.concurrent.Future

Couldn't actually think of any other useful operations to perform on BlockingSingle - in comparison to BlockingObservable, there's not much to this class (at the moment).

hyleung
hyleung reviewed Sep 2, 2015
View reviewed changes
src/test/java/rx/observables/BlockingSingleTest.java
}


private static class ThrowableWithCause extends BaseMatcher<Throwable> {
Copy link
Author

Choose a reason for hiding this comment

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

Sorry, this is gross - what I wanted to do was (in testSingleErrorChecked()) use exception.expectCause(...) from Junit 4.11, but didn't want to touch the version of that library (I tried, to upgrade, but there are a half a dozen failures in the test suite that occur).

Could get rewrite the test to try/catch and to the assertion on the caught exception, I suppose.

@hyleung
Copy link
Author

hyleung commented Sep 2, 2015

Hmmm...got a test failure that I can't reproduce locally: (fixed by #3285)

rx.observables.SyncOnSubscribeTest > testSubscribeOn FAILED
    java.lang.AssertionError: expected:<4> but was:<3>
        at org.junit.Assert.fail(Assert.java:93)
        at org.junit.Assert.failNotEquals(Assert.java:647)
        at org.junit.Assert.assertEquals(Assert.java:128)
        at org.junit.Assert.assertEquals(Assert.java:472)
        at org.junit.Assert.assertEquals(Assert.java:456)
        at rx.observables.SyncOnSubscribeTest.testSubscribeOn(SyncOnSubscribeTest.java:734

@akarnokd akarnokd mentioned this pull request Sep 2, 2015
Merged
@stealthcode
Copy link

This test failure should be resolved now.

@artem-zinnatullin artem-zinnatullin mentioned this pull request Oct 8, 2015
Closed
@abersnaze
Copy link
Contributor

@hyleung is this ready to be reviewed/merged?

@hyleung
Copy link
Author

hyleung commented Oct 9, 2015

@abersnaze Not quite yet. There are a couple of things from @artem-zinnatullin's PR (#3416) that I want to pull in (splitting out a separate BlockingUtils class to hold awaitComplete, for example) Also, I'm going to rebase it against head of 1.x since it's been about a month...

@hyleung hyleung changed the title Implements BlockingSingle 1.x: Implements BlockingSingle Oct 9, 2015
hyleung
hyleung reviewed Oct 9, 2015
View reviewed changes
src/main/java/rx/observables/BlockingSingle.java
* @return a {@link Future} that returns the value
*/
public Future<T> toFuture() {
return BlockingOperatorToFuture.toFuture(single.toObservable());
Copy link
Author

Choose a reason for hiding this comment

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

Is this "appropriate"? Could add BlockingOperatorToFuture.toFuture(Single) - wonder if it would/should behave any differently.

@hyleung
Copy link
Author

hyleung commented Oct 10, 2015

@abersnaze the PR is ready for review, btw. I'll keep the commits separate as I incorporate the feedback and squash it down at the end.

artem-zinnatullin
artem-zinnatullin reviewed Oct 10, 2015
View reviewed changes
src/main/java/rx/Single.java
@@ -1801,6 +1802,10 @@ public void onNext(T t) {
return lift(new OperatorTimeout<T>(timeout, timeUnit, asObservable(other), scheduler));
}

public final BlockingSingle<T> toBlocking() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Javadoc + @Experimental, you can grab javadoc from my PR.

@artem-zinnatullin
Copy link
Contributor

Maybe move BlockingSingle from rx.observables to rx.singles?

@hyleung
Copy link
Author

hyleung commented Oct 10, 2015

@artem-zinnatullin re. rx.singles - that would be a new package then? Don't have a particular option either way, tbh - anybody else? Ah, I see what you're getting at....moved to rx.singles. Which other operators are you thinking of adding to Single, btw?

artem-zinnatullin
artem-zinnatullin reviewed Oct 10, 2015
View reviewed changes
src/test/java/rx/internal/util/BlockingUtilsTest.java
import rx.schedulers.Schedulers;

/**
* Created with IntelliJ IDEA.
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove this header please, we have git history :)

artem-zinnatullin
artem-zinnatullin reviewed Oct 10, 2015
View reviewed changes
src/test/java/rx/observables/BlockingSingleTest.java
blockingSingle.value();
fail("Expecting an exception to be thrown");
} catch (Exception caughtException) {
assertNotNull(caughtException);
Copy link
Contributor

Choose a reason for hiding this comment

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

It's impossible :) You can remove this check.

@artem-zinnatullin
Copy link
Contributor

LGTM, just a few nits left 👍

@hyleung hyleung force-pushed the blocking-single branch 3 times, most recently from 89fcbfb to d80d70a Compare October 15, 2015 04:33
stealthcode
stealthcode reviewed Nov 10, 2015
View reviewed changes
src/main/java/rx/singles/BlockingSingle.java
*
* @return the value emitted by this {@code BlockingSingle}
*/
public T value() {
Copy link

Choose a reason for hiding this comment

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

Please add @Experimental to public methods.

@stealthcode
Copy link

I'm ready to merge this as long as we can add the necessary @Experimental annotations and rebase onto 1.x.

@hyleung
Implements BlockingSingle
d55cd40 
This commit adds BlockingSingle, the blocking version of rx.Single.

BlockingSingle has the following methods:

i `from(Single)` -- factory method for creating a `BlockingSingle` from a
`Single`
- `get()` -- returns the value emitted from the Single
- `get(Func1<T,Boolean> predicate)` -- returns the value if it matches
  the provided predicate
- `toFuture()` -- returns a `java.util.concurrent.Future`

Adds Single.toBlocking
@hyleung
Copy link
Author

hyleung commented Nov 11, 2015

@stealthcode done!

@artem-zinnatullin
Copy link
Contributor

👍

@akarnokd
Copy link
Member

👍

Thanks for contributing.

akarnokd added a commit that referenced this pull request Nov 11, 2015
@akarnokd
Merge pull request #3286 from hyleung/blocking-single
4b29429 
1.x: Implements BlockingSingle
@akarnokd akarnokd merged commit 4b29429 into ReactiveX:1.x Nov 11, 2015
@hyleung hyleung deleted the blocking-single branch November 12, 2015 05:28
@hyleung hyleung mentioned this pull request Nov 12, 2015
Closed
@DavidMGross DavidMGross mentioned this pull request Dec 2, 2015
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
Discussion Enhancement
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@hyleung @stealthcode @abersnaze @artem-zinnatullin @akarnokd