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 additional extension methods for android-archcomponents #348

Merged
merged 10 commits into from May 14, 2019

Conversation

ShaishavGandhi
Copy link
Collaborator

Part of #347

I've added an additional TestKotlinActivity in the sample just to test out the compilation of extension methods.

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.

Let's add one for View as well in this PR

* subscription will be disposed.
*/
@CheckReturnValue
inline fun <T> Flowable<T>.autoDisposable(lifecycleOwner: LifecycleOwner, untilEvent: Event): FlowableSubscribeProxy<T> =
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 default the untilEvent to something sensible and then just have one overload?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Can we really default an untilEvent reliably for everyone? Usually the untilEvent is inferred from when in the lifecycle you subscribe. However that inference logic is private in AndroidLifecycleScopeProvider.

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 be fine with it package private

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@ShaishavGandhi
Copy link
Collaborator Author

ShaishavGandhi commented May 13, 2019

Why not put this in the sample?

Thought about it but I meant to keep that file as a dumping ground for testing Kotlin compatibility. When I add the View extensions, that can also be tested in the same file. Something on the lines on KotlinSourceCompatibilityTest from OkHttp.

@ZacSweers
Copy link
Collaborator

Works for me!

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.

LGTM! Couple nits

One thing I think we should check - what happens in the case of, say, an AppCompatActivity that implements LifecycleOwner and ScopeProvider?

@@ -31,7 +33,7 @@ import java.util.concurrent.TimeUnit
* Test Activity class to verify compilation of various extension functions.
*/
@Ignore("Since it's only used to verify compilation of the extension functions")
class TestKotlinActivity : AppCompatActivity() {
class TestKotlinActivity : AppCompatActivity(), ScopeProvider {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Just to doubly confirm - it doesn't get confused about ambiguous types here if targeting this now?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No, it doesn't. Basically, it will always just resolve to one of those extension functions. It will ask you for one of them. If you're overloading with the untilEvent, it will always resolve to the one with LifecycleOwner.

@ZacSweers
Copy link
Collaborator

Couple nits but LGTM!

@ShaishavGandhi ShaishavGandhi merged commit 5dee465 into master May 14, 2019
@ShaishavGandhi ShaishavGandhi deleted the sg/arch-components-extension branch May 14, 2019 22:12
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

2 participants