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

Create extension functions that start addressing #103 #109

Closed
wants to merge 1 commit into from

Conversation

chris-horner
Copy link
Contributor

Making this as a first step to addressing #103.

I've only created extension functions for now, as calls like Observable.zip() are going to be hard to overload neatly (since Kotlin doesn't support static extension functions).

@stepango
Copy link
Collaborator

Looks good, but please mark all methods as inline.

@chris-horner
Copy link
Contributor Author

Unfortunately marking the functions as inline requires marking the lambda parameters as noinline, which I believe obviates the benefit?

@stepango
Copy link
Collaborator

stepango commented Mar 31, 2017

you could use crossinline modifier like

inline fun <T, U, R> Maybe<T>.zipWith(other: MaybeSource<out U>, crossinline zipper: (T, U) -> R): Maybe<R>
        = zipWith(other, BiFunction { t1, t2 -> zipper(t1, t2) })

@JakeWharton
Copy link
Member

JakeWharton commented Mar 31, 2017 via email

@chris-horner
Copy link
Contributor Author

Hmmm. Apparently not?

Am I missing something?

@chris-horner
Copy link
Contributor Author

Learnt new things about SAM adapters today! Changing to this style fixes the issue.

inline fun <T, U, R> Observable<T>.withLatestFrom(other: ObservableSource<U>, crossinline combiner: (T, U) -> R): Observable<R>
    = withLatestFrom(other, BiFunction<T, U, R> { t, u -> combiner.invoke(t, u)  })

PR updated.

@thomasnield
Copy link
Collaborator

thomasnield commented Apr 6, 2017

Cool, so does this mean we can use the same signatures from a usage standpoint? Can the inline and crossinline enable that?

@thomasnield
Copy link
Collaborator

Ill take a look at this today. Sorry been quiet, trying to meet deadlines at work.

@thomasnield
Copy link
Collaborator

Looking at this now. Started building an application at work and this has been driving me crazy. I will help getting all the extensions in if this approach works...

@thomasnield
Copy link
Collaborator

Wow nice, that actually works.

package io.reactivex.rxkotlin

import io.reactivex.Observable
import java.util.concurrent.TimeUnit

fun main(args: Array<String>) {
    val source1 = Observable.interval(1, TimeUnit.SECONDS)
    val source2 = Observable.interval(300, TimeUnit.MILLISECONDS)

    source1.withLatestFrom(source2) { x,y -> x to y }
            .subscribe(::println)

    Thread.sleep(10000)
}

I think I have an idea for the static factories. What if we were to create an ObservableKt and FlowableKt objects with zip(), combineLatest(), etc...?

@chris-horner
Copy link
Contributor Author

The ObservableKt approach may be our best bet for now. I like it more than defining some loose factory functions like zipObservables(). If KT-14984 is ever addressed then it's relatively easy for consumers to lop the Kt off the end of the object name and then they're using the official method.

@thomasnield
Copy link
Collaborator

@chris-horner excellent, let's move forward with this then. I propose moving everything related to SAM fixes to ObservableKt and FlowableKt, and have extensions reside next to the static factories. We may need to create SingleKt and MaybeKt as well.

@thomasnield
Copy link
Collaborator

@stepango where do you think we should put these development efforts? A separate branch in the main repository? Or work against Chris'?

@stepango
Copy link
Collaborator

@thomasnield @chris-horner yeah idea sounds good, let's make a separate branch for it

@thomasnield
Copy link
Collaborator

thomasnield commented Apr 19, 2017

I'm liking this ObservableKt approachs. Feels much cleaner than I thought it would be:

 val newSSIMData = ObservableKt.combineLatest(newSSIMFile.toObservable(), selectedDateRange) { ssim, dates ->
        SSIMRecord.forSSIMFile(ssim)
                .toList().subscribeOn(Schedulers.io())
    }.switchMap { it.toObservable() }
     .replay(1)
     .autoConnect()

Having some trouble getting the Function3 overload of combineLatest() to compile:

object ObservableKt {
    inline fun <T1,T2,R> combineLatest(source1: Observable<T1>, source2: Observable<T2>, crossinline combineFunction: (T1,T2) -> R) =
            Observable.combineLatest(source1, source2,
                    BiFunction<T1, T2, R> { t1, t2 -> combineFunction(t1,t2) })!!

    inline fun <T1,T2,T3,R> combineLatest(source1: Observable<out T1>, source2: Observable<out T2>, source3: Observable<out T3>,
                                          crossinline combineFunction: (T1, T2, T3) -> R) =
            Observable.combineLatest(source1, source2, source3, Function3<T1,T2,T3,R> { t1,t2,t3 -> combineFunction.invoke(t1,t2,t3) })!!
}

@thomasnield thomasnield self-requested a review April 19, 2017 16:48
@thomasnield
Copy link
Collaborator

Got it, never mind.

object ObservableKt {
    inline fun <T1,T2,R> combineLatest(source1: Observable<T1>, source2: Observable<T2>, crossinline combineFunction: (T1,T2) -> R) =
            Observable.combineLatest(source1, source2,
                    BiFunction<T1, T2, R> { t1, t2 -> combineFunction(t1,t2) })!!

    inline fun <T1,T2,T3,R> combineLatest(source1: Observable<T1>, source2: Observable<T2>, source3: Observable<T3>, crossinline combineFunction: (T1,T2, T3) -> R) =
            Observable.combineLatest(source1, source2,source3,
                    io.reactivex.functions.Function3<T1, T2, T3, R> { t1: T1, t2: T2, t3: T3 -> combineFunction(t1,t2, t3) })!!
}

@thomasnield
Copy link
Collaborator

I created a new branch and ported over @chris-horner's work and added some more operators.

https://github.com/ReactiveX/RxKotlin/tree/sam-adapters

Please help and contribute anyone who wants to get involved. I will try to add more each day until it is done.

@thomasnield
Copy link
Collaborator

Closing in favor of #114

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