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

Add subscribeBy extensions for proxy classes. #127

Merged
merged 5 commits into from
Nov 10, 2017
Merged

Add subscribeBy extensions for proxy classes. #127

merged 5 commits into from
Nov 10, 2017

Conversation

ajalt
Copy link
Contributor

@ajalt ajalt commented Nov 7, 2017

As we discussed at Droidcon, since the SubscribeProxy classes don't extend the
observable class they proxy to, extension functions written for the
observable class can't be used.

One commonly used set of extensions is RxKotlin's subscribeBy. This
commit adds copies of those extensions that work on SubscribeProxies.

Unfortunately, since the SubscribeProxy classes don't extend the
observable class they proxy to, extension functions written for the
observable class can't be used.

One commonly used set of extensions is RxKotlin's subscribeBy. This
commit adds copies of those extensions that work on SubscribeProxies.
@CLAassistant
Copy link

CLAassistant commented Nov 7, 2017

CLA assistant check
All committers have signed the CLA.

@ZacSweers
Copy link
Collaborator

While I like the idea, I think this would be better served as a recipe/example in the sample app rather than the public supported API. It's simple enough that people can add it in their own projects as they see fit, and keep the kotlin artifacts as lean as possible. What do you think?

@ajalt
Copy link
Contributor Author

ajalt commented Nov 8, 2017

I think that keeping the artifacts small is a generally good idea, and these extensions could be considered outside the scope of the library. It seems like these particular extensions might be worth including due to the fact that they are the only ones in RxKotlin that don't work with AutoDispose, and they are fairly widely used. But it's up to you.

@ZacSweers
Copy link
Collaborator

My gut says it's better to make these a separate thing - erring on the side of restricting the API surface area in autodispose itself. If you want to put them in the sample though, I'd be happy to merge that and point to them as a recipe for RxKotlin users. This also sounds like a good library idea for you if you wanted to maintain it yourself :)

@ajalt
Copy link
Contributor Author

ajalt commented Nov 8, 2017

Ok, I moved the extensions to the sample recipes and updated the activity to use them. Is that what you had in mind?

Copy link
Collaborator

@ZacSweers ZacSweers left a comment

Choose a reason for hiding this comment

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

Yep the way you've structured it works perfect. Left some comments and nits, but overall really like having an example like this here

@@ -0,0 +1,54 @@
package com.uber.autodispose.recipes

import com.uber.autodispose.*
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: no wildcards

private val onErrorStub: (Throwable) -> Unit = { RxJavaPlugins.onError(OnErrorNotImplementedException(it)) }
private val onCompleteStub: () -> Unit = {}


Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: single blank line

import io.reactivex.exceptions.OnErrorNotImplementedException
import io.reactivex.plugins.RxJavaPlugins

private val onNextStub: (Any) -> Unit = {}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's add a file doc above this that explains how this is an example of how you can tack on extension functions to the proxy interface

/*
 * Stuff!
 */

import io.reactivex.plugins.RxJavaPlugins

private val onNextStub: (Any) -> Unit = {}
private val onErrorStub: (Throwable) -> Unit = { RxJavaPlugins.onError(OnErrorNotImplementedException(it)) }
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd like to propose diverging from how RxKotlin does this. The reason is that RxJava actually does special handling for missing error implementations via LambdaConsumerIntrospection, that supplying this would not catch. I'd say for the cases where only a next/success/complete consumer/action is passed in, let's call through to the unhandled one in rxjava. Will comment inline with specifics

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hm, interesting. I added RxKotlin's current implementation before LambdaConsumerIntrospection was in place, and it matched RxJava's behavior at the time. I'll look into your PR in detail later, but do you think RxKotlin should also be updated?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah I'd say it probably should. Would be a non-breaking change, but it would allow rxjava to properly expose the introspection details

onError: (Throwable) -> Unit = onErrorStub,
onComplete: () -> Unit = onCompleteStub,
onNext: (T) -> Unit = onNextStub
): Disposable = subscribe(onNext, onError, onComplete)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Adjust for LambdaConsumerIntrospection

if (onError === onErrorStub && onComplete === onCompleteStub) {
  return subscribe(onNext)
} else {
  return subscribe(onNext, onError, onComplete)
}

onError: (Throwable) -> Unit = onErrorStub,
onComplete: () -> Unit = onCompleteStub,
onNext: (T) -> Unit = onNextStub
): Disposable = subscribe(onNext, onError, onComplete)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Adjust for LambdaConsumerIntrospection

if (onError === onErrorStub && onComplete === onCompleteStub) {
 return subscribe(onNext)
} else {
 return subscribe(onNext, onError, onComplete)
}

fun <T : Any> SingleSubscribeProxy<T>.subscribeBy(
onError: (Throwable) -> Unit = onErrorStub,
onSuccess: (T) -> Unit = onNextStub
): Disposable = subscribe(onSuccess, onError)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Adjust for LambdaConsumerIntrospection

if (onError === onErrorStub) {
 return subscribe(onSuccess)
} else {
 return subscribe(onSuccess, onError)
}

onError: (Throwable) -> Unit = onErrorStub,
onComplete: () -> Unit = onCompleteStub,
onSuccess: (T) -> Unit = onNextStub
): Disposable = subscribe(onSuccess, onError, onComplete)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Adjust for LambdaConsumerIntrospection

if (onError === onErrorStub && onComplete === onCompleteStub) {
 return subscribe(onSuccess)
} else {
 return subscribe(onSuccess, onError, onComplete)
}

fun CompletableSubscribeProxy.subscribeBy(
onError: (Throwable) -> Unit = onErrorStub,
onComplete: () -> Unit = onCompleteStub
): Disposable = subscribe(onComplete, onError)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Adjust for LambdaConsumerIntrospection

if (onError === onErrorStub) {
 return subscribe(onComplete)
} else {
 return subscribe(onComplete, onError)
}

@@ -45,7 +46,7 @@ class KotlinActivity : AppCompatActivity() {
Observable.interval(1, TimeUnit.SECONDS)
.doOnDispose { Log.i(TAG, "Disposing subscription from onCreate()") }
.autoDisposeWith(scopeProvider)
.subscribe { num -> Log.i(TAG, "Started in onCreate(), running until onDestroy(): " + num) }
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: let's keep the num, as it was an intentionally more specific name

Copy link
Collaborator

@ZacSweers ZacSweers left a comment

Choose a reason for hiding this comment

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

One more nit, but for readability I'd prefer the method bodies be filled out. The IDE likes to yolo suggest inlining All The Things, but in this case I think it hurts readability

@ajalt
Copy link
Contributor Author

ajalt commented Nov 9, 2017

I'm not sure I understand what you mean by filling out method bodies?

@ZacSweers
Copy link
Collaborator

As in instead of:

fun <T : Any> ObservableSubscribeProxy<T>.subscribeBy(
        onError: (Throwable) -> Unit = onErrorStub,
        onComplete: () -> Unit = onCompleteStub,
        onNext: (T) -> Unit = onNextStub
): Disposable =
        if (onError === onErrorStub && onComplete === onCompleteStub) {
            subscribe(onNext)
        } else {
            subscribe(onNext, onError, onComplete)
        }

Do this

fun <T : Any> ObservableSubscribeProxy<T>.subscribeBy(
        onError: (Throwable) -> Unit = onErrorStub,
        onComplete: () -> Unit = onCompleteStub,
        onNext: (T) -> Unit = onNextStub
): Disposable {
  return if (onError === onErrorStub && onComplete === onCompleteStub) {
    subscribe(onNext)
  } else {
    subscribe(onNext, onError, onComplete)
  }
}

Copy link
Collaborator

@ZacSweers ZacSweers left a comment

Choose a reason for hiding this comment

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

Thanks!!

@ZacSweers ZacSweers merged commit 1817cca into uber:master Nov 10, 2017
@ZacSweers ZacSweers mentioned this pull request Jul 1, 2018
5 tasks
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.

3 participants