Implemented: TakeWhile

[mairbek]

No description provided.

[benjchristensen]

Hi @mairbek, thank you for contributing.

I'm going to checkout your branch to review this and will comment more (or merge if nothing needs to change) once I've done so.

Talk to you soon ...

[benjchristensen]

@mairbek I have reviewed the code and it looks good.

The missing part is the addition of methods to rx.Observable.

The 2 methods you'd need to model after are:

https://github.com/Netflix/RxJava/blob/master/rxjava-core/src/main/java/rx/Observable.java#L1324
https://github.com/Netflix/RxJava/blob/master/rxjava-core/src/main/java/rx/Observable.java#L2212

Note that there is both a static and instance method.

Since takeWhile includes a Func it will also need to support an overload with Object such as this:

-> typed: https://github.com/Netflix/RxJava/blob/master/rxjava-core/src/main/java/rx/Observable.java#L2212
-> non-typed: https://github.com/Netflix/RxJava/blob/master/rxjava-core/src/main/java/rx/Observable.java#L2269

So you would end up with:

static Observable.takeWhile(final Observable items, final Func1<T, Boolean> predicate)
static Observable.takeWhile(final Observable items, final Func2<T, Integer, Boolean> predicate)
static Observable.takeWhile(final Observable items, final Object predicate)
instance Observable.takeWhile(final Func2<T, Integer, Boolean> predicate)
instance Observable.takeWhile(final Func1<T, Boolean> predicate)
instance Observable.takeWhile(final Object predicate)

One challenge with supporting dynamic languages is that overloading can get tricky and the implementation of takeWhile as it currently stands has overloads with Func1 and Func2. This means that the overload with Object will convert to FuncN, but we won't know which of the Func1 or Func2 methods to send to.

Thus, you may want to look at changing the OperationTake to use only Func2 for the takeWhile methods.

When adding methods involving Func* overloads I then add unit tests for at least one of the dynamic languages to make sure it's all working when going via the "Object" overloads.

For example: https://github.com/Netflix/RxJava/blob/master/language-adaptors/rxjava-groovy/src/main/java/rx/lang/groovy/GroovyAdaptor.java#L85

Do you want to update this pull request to include the rx.Observable methods?

[mairbek]

Thus, you may want to look at changing the OperationTake to use only Func2 for the takeWhile methods.

I suppose people would mostly look for the Func1 method and use cases when you need Func2 function are pretty rare.
I'm going to name takeWhile with Func2 as takeWhileWithIndex (not sure if this is a good name for this) to avoid overloading, what do you think?

[benjchristensen]

I need to spend some time playing with this as I'd like to try and find a way that doesn't require a different name.

I haven't forgotten this but need to finish some other things then will come back to this.

[benjchristensen]

This is for issue #87

[benjchristensen]

This can no longer be automatically merged so I'm manually merging it into my local branch to play with and will either respond back with comments or submit with your commits in another pull request based off of yours.

[benjchristensen]

To allow us to avoid having takeWhileWithIndex (and just have takeWhile) is it bad to require all closures to pass in both arguments and just ignore the index if they don't want it?

Observable.takeWhile(Observable.toObservable(1, 2, 3), { x, i -> i < 2}).subscribe({ result -> a.received(result)});

(-> (rx.Observable/toObservable [1 2 3]) (.takeWhile (fn [x i] (< x 2))) (.subscribe (fn [arg] (println arg))))

versus

Observable.takeWhile(Observable.toObservable(1, 2, 3), { x -> x < 2}).subscribe({ result -> a.received(result)});

(-> (rx.Observable/toObservable [1 2 3]) (.takeWhile (fn [x] (< x 2))) (.subscribe (fn [arg] (println arg))))

Asking around for opinions ...

[benjchristensen]

I have completed the merge in pull request #146

I'll close this one out now.

I stuck with the proposed design by @mairbek for the pull request and it has takeWhile and takeWhileWithIndex since it seems the common use case would be takeWhile so it keeps the closure implementations simple with a single arg.

Of course since we're version 0.5 we can change this decision prior to 1.0 if there are strong opinions otherwise.

[benjchristensen]

Thank you @mairbek, great implementation. I appreciate how clean and well tested your code is and that you strive to comply with the style of the project.

[daveray]

I prefer takeWhile and takeWhileIndexed. Two separate names is more explicit.

[benjchristensen]

Thanks Dave, I also discussed with Jafar who agreed so we're good with the takeWhilte/takeWhileIndexed naming convention and I'll leave the code as committed.