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 "First" and "FirstOrDefault" operations #357

Merged
merged 4 commits into from
Sep 10, 2013

Conversation

jmhofer
Copy link
Contributor

@jmhofer jmhofer commented Sep 8, 2013

This PR builds upon the skipWhile PR (#355) which makes implementing the two first variants extremely easy.

I changed firstOrDefault slightly from what Rx.NET does by explicitly requiring a default value as parameter of the function call. I don't know what default value I should return otherwise, except null (and imho this would be pretty useless).

This PR addresses issue #44.

@cloudbees-pull-request-builder

RxJava-pull-requests #248 SUCCESS
This pull request looks good

* source Observable completes without emitting a single item.
* @see <a href="http://msdn.microsoft.com/en-us/library/hh229177%28v=vs.103%29.aspx">MSDN: Observable.First</a>
*/
public Observable<T> first() {
Copy link
Member

Choose a reason for hiding this comment

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

The naming convention here is different than last and takeLast so wondering if we should change to match.

The last operator is blocking so on BlockingObservable. This first operator seems to be similar to single and blocking last.

Thus I wonder if we should have takeFirst instead of first? This is different than Rx.Net but would clean up the same difference in convention that it has.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd expect the names to be first and last whether blocking or not. Also, should BlockingObservable really extend Observable? Seems like a potential source of confusion.

Copy link
Member

Choose a reason for hiding this comment

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

It doesn't extend from Observable any longer.

Copy link
Contributor

Choose a reason for hiding this comment

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

Cool. I'm behind the times :)

Copy link
Member

Choose a reason for hiding this comment

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

So takeFirst/takeLast/ or first/last ... or both aliased?

Until we split off BlockingObservable last and takeLast meant different things, we could now have last exist on both but with different return types.

@jmhofer
Copy link
Contributor Author

jmhofer commented Sep 9, 2013

In Scala, first would be called head... - do we have some kind of skipFirst (aka tail) too?

I'm okay with both naming schemes, with a slight preference for leaving the takes away.

I was also playing around with firstOrElse instead of firstOrDefault, btw. - The orElse naming scheme is quite common in Scala, and the default is not really much of a default anyway when you have to explicitly specify it in a parameter. I left it as is because of the .NET naming scheme, though.

@benjchristensen
Copy link
Member

I need to look at what Java 8 is calling these things, as that is the more important naming convention for the core lib to match.

@benjchristensen
Copy link
Member

Java 8 uses findFirst which doesn't seem all that helpful: http://download.java.net/jdk8/docs/api/java/util/stream/Stream.html#findFirst()

@benjchristensen benjchristensen merged commit 98cccec into ReactiveX:master Sep 10, 2013
rickbw pushed a commit to rickbw/RxJava that referenced this pull request Jan 9, 2014
…into operator-first-merge

Conflicts:
	rxjava-core/src/test/java/rx/ObservableTests.java

This merges pull request ReactiveX#357

Also aliased first with takeFirst to match takeLast.
billyy pushed a commit to billyy/RxJava that referenced this pull request Jan 13, 2014
…into operator-first-merge

Conflicts:
	rxjava-core/src/test/java/rx/ObservableTests.java

This merges pull request ReactiveX#357

Also aliased first with takeFirst to match takeLast.
benjchristensen added a commit to ReactiveX/RxScala that referenced this pull request Aug 19, 2014
…into operator-first-merge

Conflicts:
	rxjava-core/src/test/java/rx/ObservableTests.java

This merges pull request ReactiveX/RxJava#357

Also aliased first with takeFirst to match takeLast.
benjchristensen added a commit to ReactiveX/RxScala that referenced this pull request Aug 19, 2014
…into operator-first-merge

Conflicts:
	rxjava-core/src/test/java/rx/ObservableTests.java

This merges pull request ReactiveX/RxJava#357

Also aliased first with takeFirst to match takeLast.
jihoonson pushed a commit to jihoonson/RxJava that referenced this pull request Mar 6, 2020
…bscriber is is not permitted to subscribe, the subscription must be canceled and the ResilienceBaseSubscriber must call onSubscribe on the target Subscriber with an EmptySubscription followed by a call to onError with the supplied error. (ReactiveX#452)

Issue ReactiveX#357: Fixed a bug in ResilienceBaseSubscriber. If a subscriber is is not permitted to subscribe, the subscription must be canceled and the ResilienceBaseSubscriber must call onSubscribe on the target Subscriber with an EmptySubscription followed by a call to onError with the supplied error. The bug was that the ResilienceBaseSubscriber cancelled it's own subscription as well.
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.

None yet

4 participants