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

Need inclusive predicate variant of takeUntil/takeWhile #1649

Closed
samueltardieu opened this issue Aug 31, 2014 · 36 comments
Closed

Need inclusive predicate variant of takeUntil/takeWhile #1649

samueltardieu opened this issue Aug 31, 2014 · 36 comments
Milestone

Comments

@samueltardieu
Copy link
Contributor

It has been two times I've been missing a takeUntil(filter) operator on observables. The last time was in cgeo while implementing a low-power mode: I want to receive location updates through an observable and stop as soon as a location arrives with a precision of 20m or less, but I want this location to be returned. Using takeWhile(!filter) would drop the matching item.

Of course, this can be worked around, but I was surprised not to get this operator which is the dual of takeWhile. Would you consider a contribution adding it?

@benjchristensen
Copy link
Member

Good question. I'd like to find out from @headinthebox what the canonical solution is to this as Haskell, various Rx flavors and others either must have or need this solution as well.

The takeUntil(filter) solution seems like it is a reasonable approach. My only question is about the semantics as I could interpret "until" either way (inclusive or exclusive).

@benjchristensen
Copy link
Member

/cc @spodila who also needs a solution to this

@benjchristensen benjchristensen changed the title takeUntil would complement takeWhile in an useful way Need inclusive predicate variant of takeUntil/takeWhile Sep 2, 2014
@spodila
Copy link
Contributor

spodila commented Sep 2, 2014

Nice. I could use that. Thank you.

@headinthebox
Copy link
Contributor

We cannot add every possible operator that people need in the library. Instead we must make sure you can achieve waht you want by composing a set of orthogonal primitive operators. In this case the solution is as follows:

object MainScala {

  def main(args: Array[String]): Unit = {

    val xs = Observable.items(0,1,2,3,4)

    val ys = xs.takeWhile(x => x < 3)
    ys.subscribe(y => println(y)) // 0,1,2

    val zs = xs.publish[Int]((xs: Observable[Int]) => xs.takeUntil(xs.dropWhile(x => x < 3)))
    zs.subscribe(z => println(z)) // 0,1,2,3
    readLine()
  }
}

@spodila
Copy link
Contributor

spodila commented Sep 2, 2014

Maybe I am missing something...
I don't see dropWhile() in 0.20.3. Is it already being added?
That makes it look a bit convoluted, but looks like my use case can work with it.

@samueltardieu
Copy link
Contributor Author

@headinthebox Is there a guarantee that your example will work in every case? I may be missing something, but where in the documentation is it described that the xs.dropWhile(…) observable will consider the respective elements of xs before or after xs.takeUntil(…) one? Isn't there a race condition here? Is it guaranteed on every scheduler?

In your example, couldn't xs.dropWhile(x => x < 3) emit "4" only after xs.takeUntil(…) have already let the "4" pass?

@zsxwing
Copy link
Member

zsxwing commented Sep 3, 2014

I don't see dropWhile() in 0.20.3. Is it already being added?

dropWhile is here: https://github.com/ReactiveX/RxScala/blob/0.x/src/main/scala/rx/lang/scala/Observable.scala#L1981

@samueltardieu
Copy link
Contributor Author

@headinthebox Your example with RxJava 0.20.4 gives to me "0 1 2 0 1 2", not "0 1 2 0 1 2 3" as shown in your comments. Your code seems to be the strict equivalent to takeWhile (drop as soon as the negation of the condition is met), which does not do what is requested.

The proposed takeUntil(_ >= 3) would properly give "0 1 2 3".

@headinthebox
Copy link
Contributor

I'm without a proper computer right now, but did you run exactly the same snippet using RxScala?

@samueltardieu
Copy link
Contributor Author

Yup, I only added the import:

import rx.lang.scala._

object MainScala {

  def main(args: Array[String]): Unit = {

  val xs = Observable.items(0, 1, 2, 3, 4)

    val ys = xs.takeWhile(x => x < 3)
    ys.subscribe(y => println(y)) // 0,1,2

    val zs = xs.publish[Int]((xs: Observable[Int]) => xs.takeUntil(xs.dropWhile(x => x < 3)))
    zs.subscribe(z => println(z)) // 0,1,2,3
    readLine()
  }

build.sbt:

scalaVersion := "2.11.2"

libraryDependencies += "com.netflix.rxjava" % "rxjava-scala" % "0.20.4"

And run:

% sbt
[info] Set current project to x (in build file:/tmp/x/)
> run
[info] Updating {file:/tmp/x/}x...
[info] Resolving jline#jline;2.12 ...
[info] Done updating.
[info] Running MainScala 
0
1
2
0
1
2

[success] Total time: 34 s, completed Sep 16, 2014 7:41:26 PM
>

But from what I read, this is expected, as takeUntil will stop as soon as the dropWhile fires, while we want to keep one more element. Maybe adding a tail after dropWhile would get the right one, but then comes my former question about ordering guarantees, and the code becomes convoluted.

@samueltardieu
Copy link
Contributor Author

@headinthebox Any news on this one?

@headinthebox
Copy link
Contributor

Was blocked because of broken scala build.

@headinthebox
Copy link
Contributor

Yup, the tail is missing. There is no issue with reordering. Using .publish and takeUntil on yourself is a very common pattern that more people should use. For example you can also define switch that same way.

import rx.lang.scala._

object MainScala {
  def main(args: Array[String]): Unit = {
  val xs = Observable.items(0, 1, 2, 3, 4)
  val zs = xs.publish[Int]((xs: Observable[Int]) => xs.takeUntil(xs.dropWhile(x => x < 3).tail))
  zs.subscribe(z => println(z)) // 0,1,2,3
  readLine()
}

@samueltardieu
Copy link
Contributor Author

Ok. But switch is defined nevertheless because it is convenient, not because it is needed, as do many other operators, publishLast comes to mind. I still think an inclusive takeUntil would be convenient in common situations (such as the one I described originally, which is stopping right after you get a matching value), and is quite contrived to define (especially when you are writing Java 7 code, which is the case for Android developers at this time).

I can live without it (and do at this time), but I still think RxJava would be better with those inclusive versions.

@headinthebox
Copy link
Contributor

The point of Rx is to be composable. We cannot anticipate every single operator that people want to use (_). There are already _way too many* operators in the library, so the default answer is "no" until we seen a pattern appearing in a majority of code (and conversely, we should deprecate operators that can be defined in terms of other and are seldomly used).

@samueltardieu
Copy link
Contributor Author

So let's start with a count of 1 for takeUntil :) https://github.com/cgeo/cgeo/blob/master/main/src/cgeo/geocaching/utils/RxUtils.java#L108

@benjchristensen
Copy link
Member

One problem with using publish is it does break backpressure as it introduces multicasting. It is a fine solution for hot streams but not desirable for cold ones.

I use it for example on a buffered denounce, but backpressure is not relevant in that case. Take operators on the other hand should be usable without breaking backpressure.

@benjchristensen benjchristensen added this to the 1.0.x milestone Oct 7, 2014
@benjchristensen
Copy link
Member

Putting on 1.0.x as this is not a blocker for 1.0 but I want to continue considering and discussing this. Also opened #1732 related to this.

@benjchristensen
Copy link
Member

What about takeWhileInclusive as a name for this?

The issue is that takeUntil(predicate) is the same signature as takeWhile(predicate) and the only difference would be inclusive vs exclusive but the names are ambiguous.

@headinthebox and I are good for adding this as it is a common need, but we're struggling with naming.

The API would look like this:

  • takeUntil(Observable)
  • takeWhile(predicate)
  • takeWhileInclusive(predicate)

What better names (starting with take) exist for this?

@samueltardieu
Copy link
Contributor Author

What about skipAfter, or terminateAfter? This (Scala syntax) does not look too strange to me:

    observable.terminateAfter(_ > 3).subscribe(…)

does not look that strange to me. terminateAfter carries the notion that the observable will be terminated right after the matching value, while skipAfter would rather mean that we skip the onNext events and propagate the onTerminated and onError.

@samueltardieu
Copy link
Contributor Author

takeWhileInclusive may be confusing, because it would take the first non-matching value. Being positive about the termination clause might be clearer, since it is it that we want to be inclusive.

@benjchristensen
Copy link
Member

So what are you proposing? takeUntil is also confusing.

@samueltardieu
Copy link
Contributor Author

I am proposing terminateAfter (see above).

@benjchristensen
Copy link
Member

terminateAfter is non-discoverable since almost everyone will be looking at take, takeUntil and takeWhile alphabetically next to each other. It is preferable to have a name starting with take*.

And the skipAfter operator implies it does not unsubscribe but just drops the values.

@samueltardieu
Copy link
Contributor Author

takeUntilInclusive may be less confusing than takeWhileInclusive, since you really want to take the values until a condition matches including the matching value (or takeUntilIncluding).

@benjchristensen
Copy link
Member

There is no "exclusive" takeUntil ... I do agree however than "until" is the right word, not "while".

Here is usage:

[start, a, b, c, stop, d, e].takeUntilXXX(x -> x == stop)
// return [start, a, b, c, stop]
[1, 2, 3, 4, 5].takeUntilXXX(x -> x == 3)
// return [1, 2, 3]
[1, 2, 3, 4, 5].takeUntilXXX(x -> x > 3)
// return [1, 2, 3, 4]

@benjchristensen
Copy link
Member

Going back to the takeUntil(predicate) signature it could be like this:

takeUntil(Observable) // unsubscribe when observable emits
takeUntil(T -> Boolean) // unsubscribe when predicate returns true (inclusive)
takeWhile(T -> Boolean) // unsubscribe when predicate returns false (exclusive)

I'm concerned with the arbitrary difference of inclusive/exclusive and similarity of signatures. The "until" suffix feels like it could be either inclusive or exclusive. takeWhile however seems to behave as expected... drops and unsubscribes once it becomes false.

@jimwhite
Copy link

How about takeWithUntil for takeUntilXXX? I'm not familiar with RxJava enough to know how much or if the inclusive sense of 'with' conflicts with its other usage here but it seems sensible enough (and short). The reason for putting 'with' in the middle is to keep the 'until' closer to the condition.

@headinthebox
Copy link
Contributor

Note that this operator is not present in any collection library I know of, hence we cannot steal an existing name.

I guess Haskell developers would use takeWhileAndThenOne = (\(x,y:_) -> x ++ [y]).span p which does not work for us ;-)

@benjchristensen benjchristensen modified the milestones: 1.0.x, 1.1 Nov 15, 2014
@benjchristensen
Copy link
Member

I hit another use case today where I'd really like to have this.

@akarnokd
Copy link
Member

akarnokd commented Feb 5, 2015

An experimental takeUntil() is now available in 1.0.5.

@akarnokd akarnokd closed this as completed Feb 5, 2015
@headinthebox
Copy link
Contributor

One way to distinguish the two versions would be takeUntil (forwards, then checks) versus untilTake (checks, the forwards). Like do-while and while.

@mxklabs
Copy link

mxklabs commented Apr 24, 2017

I second the idea of needing this operator.

I'd argue this is a basic primitive stream operation that ought not to be composed. Not including it is like Java designers arguing that you don't need a <= operator on integers because you already have < and == operators.

@akarnokd
Copy link
Member

@mxklabs how urgently do you need this operator?

@mxklabs
Copy link

mxklabs commented Apr 25, 2017

@akarnokd I need it now but can probably work around it. I just wanted to agree with the sentiment that it makes sense to have this operator.

@mxklabs
Copy link

mxklabs commented Apr 25, 2017

@akarnokd Also, take until (from what I understand from here) is not exactly what's requested. It takes two observables and terminates as soon as the second observable emits an item as opposed to an observable and a condition and keep emitting upto and including the first emit where the condition holds.

(Not sure if I am looking at the right documentation, sorry)

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

No branches or pull requests

8 participants