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 AutoDisposeContext + withScope functions #375

Merged
merged 6 commits into from
Sep 15, 2019
Merged

Conversation

ZacSweers
Copy link
Collaborator

This allows for some more idiomatic kotlin usage like so:

withScope(sourceScope) {
  someObservable.autoDispose().subscribe()
}

Copy link
Collaborator

@ShaishavGandhi ShaishavGandhi left a comment

Choose a reason for hiding this comment

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

I wish there was a way to let go of the autoDispose() and then it would read exactly like Flow but alas.

@@ -253,3 +254,48 @@ inline fun <T> ParallelFlowable<T>.autoDisposable(provider: ScopeProvider): Para
@CheckReturnValue
inline fun <T> ParallelFlowable<T>.autoDispose(provider: ScopeProvider): ParallelFlowableSubscribeProxy<T> =
this.`as`(AutoDispose.autoDisposable(provider))

/** Executes a [body] with under an [AutoDisposeContext] backed by the given [scope]. */
Copy link
Collaborator

Choose a reason for hiding this comment

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

I guess you want with or under? i.e
Executes a [body] under an ....
or
Executes a [body] with an...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

}

/**
* A context intended for use as `AutoDisposeCoroutineScope.() -> Unit` function body parameters
Copy link
Collaborator

Choose a reason for hiding this comment

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

Given that this is in the autodispose module and that you can use it with a normal ScopeProvider as well, we can probably skip the reference to coroutines.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That's a stale doc from the original PR, will fix

Copy link
Collaborator Author

@ZacSweers ZacSweers Sep 15, 2019

Choose a reason for hiding this comment

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

@ShaishavGandhi
Copy link
Collaborator

ShaishavGandhi commented Sep 15, 2019

How about adding convenience functions like:

fun withScope(coroutineScope: CoroutineScope, body: AutoDisposeContext.() -> Unit) {
  withScope(coroutineScope.toScopeProvider(), body)
}

in the coroutine-interop module?

Can do in a followup if you want.

@ZacSweers
Copy link
Collaborator Author

How about adding convenience functions

I don't like it in this case tbh. I think convenience functions make sense for discoverability and shorthands for common cases, but that case isn't either of those.

@ZacSweers ZacSweers merged commit 3e9286d into master Sep 15, 2019
@ZacSweers ZacSweers deleted the z/autoDisposeContext branch September 15, 2019 20:14
@ShaishavGandhi
Copy link
Collaborator

Given that withScope is so similar to withContext from coroutines, it'd be perplexing to not overload withScope with a CoroutineScope since coroutines users will have an instinct of using the with* functions. Just my 2 cents.

@ZacSweers
Copy link
Collaborator Author

ZacSweers commented Sep 16, 2019

withContext is not a scoping construct like this is though, whereas this is a scoping construct and not doing anything implicitly with the block passed in other than affording it extra APIs. withContext will execute your block on a different context. I thought about calling this AutoDisposeScope at first, but context in this case in reference to the context that those extension functions are available.

It's best not to think of this as having anything to do with coroutines or coroutine scope because it's unrelated functionality. The only thing in common really is that they use higher order functions with receivers :)

@ZacSweers
Copy link
Collaborator Author

Also - I think a good API boundary here is supporting AutoDispose's core types, and anything else that can be interop'd back to them should just use that. Otherwise it's a slippery slop to supporting anything that Completable and ScopeProvider can interop out to: CoroutineScope, LifecycleProvider (2 and 3), etc. If someone wants to write their own extension function to get the API you're describing, great! But otherwise I feel pretty strongly about keeping the API surface area here down to just AutoDispose's own core components.

@ShaishavGandhi
Copy link
Collaborator

Also - I think a good API boundary here is supporting AutoDispose's core types, and anything else that can be interop'd back to them should just use that.

Yeah I strongly agree with that. Given the nature of composability of Rx, we'd end up supporting lots of overloads.

The little inkling was mainly because of the similarity in names but it's only a minor inconvenience for people to just do .asScopeProvider()

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants