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

Proposing Observable.subscribeWith { ... } #17

Merged
merged 4 commits into from May 12, 2015
Merged

Proposing Observable.subscribeWith { ... } #17

merged 4 commits into from May 12, 2015

Conversation

SalomonBrys
Copy link
Contributor

I find the current subscriber syntax cumbersome. Here's an example:

myObservable.subscribe(subscriber<String>()
    .onNext { println(it) }
    .onError { it.printStackTrace() }
)

The problem I have is with subscriber<String>(). Because it creates a subscriber that is later modified with .onNext and .onError, the Kotlin compiler cannot infer it's type.
For subscriber<String>, this is not such an issue, but for subscriber<Map<String, User.Status>> this can quickly become cumbersome.

Beside, we could better use Kotlin's amazing type inference system and use a more "Kotlin-esque" way.

This PR introduces a new way of subscribing to an observable with this proposed API:

myObservable.subscribeWith {
    onNext { println(it) }
    onError { it.printStackTrace() }
}

The API is using Kotlin's type-safe builder pattern.

Let me know what you think ;)

@SalomonBrys
Copy link
Contributor Author

Travis has failed because "The command "eval git clone --depth=50 git://github.com/ReactiveX/RxKotlin.git ReactiveX/RxKotlin" failed 3 times." but this PR does pass all unit tests

@cy6erGn0m
Copy link
Contributor

You have to note that you generally don't need to create subscriber in this case. You can just do

observable.onError {.... } .subscribe { ... }

So you need this builder if you need to make subscriber and use it later and subscribe dynamically. In this case you will need to specify type anyway.
However I believe it could be good to see this declarative builder too

@SalomonBrys
Copy link
Contributor Author

Unfortunately not.

That was indeed my first try, but when using .onError {} .subscribe {}, RxJava will throw a OnErrorNotImplementedException in case of error because onError does not catch the error (only runs in case of error).

The correct syntax would be:

observable
    .onErrorResumeNext {
        it.printStackTrace()
        /* Handle the error */
        emptyObservable()
    }
    .subscribe {
        /* Handle next value */
    }

That is why I propose the syntax exposed in this PR ;)

@cy6erGn0m
Copy link
Contributor

With pull #16 subscribeWith could be implemented simply like this

public inline fun <T> Observable<T>.subscribeWith( body : FunctionSubscriber<T>.() -> FunctionSubscriber<T>) : Subscription {
    return subscribe(subscriber<T>().body())
}

@SalomonBrys
Copy link
Contributor Author

@cy6erGn0m I don't think so since, even in #16, the setter function do not set the value of the current subscriber but create a new copied subscriber with the updated function

@cy6erGn0m
Copy link
Contributor

@SalomonBrys I see, you will need dots in this case

@MarioAriasC
Copy link
Collaborator

#16 was merged. You need a rebase

@SalomonBrys
Copy link
Contributor Author

Done ;) (I did a merge, not a rebase)

@SalomonBrys
Copy link
Contributor Author

Hi there. Any news on this PR?

@MarioAriasC
Copy link
Collaborator

@cy6erGn0m any other comments on this PR?

A 👍 from me

@cy6erGn0m
Copy link
Contributor

I think we can go with it. Lets merge it and release then

MarioAriasC added a commit that referenced this pull request May 12, 2015
Proposing Observable.subscribeWith { ... }
@MarioAriasC MarioAriasC merged commit f34493f into ReactiveX:0.x May 12, 2015
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

3 participants