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

RxScala: Add the rest missing methods to BlockingObservable #1336

Merged
merged 7 commits into from
Jun 16, 2014

Conversation

zsxwing
Copy link
Member

@zsxwing zsxwing commented Jun 8, 2014

Some comments about this PR:

  • Changed the constructor from BlockingObservable[+T] private[scala] (val asJava: rx.observables.BlockingObservable[_ <: T]) to BlockingObservable[+T] private[scala] (val o: Observable[T]) so that we can implement logic in BlockingObservable.
  • Avoided to use toIterable to implement ***Option and ***OrElse because if some exception happens, we can not unsubscribe the underlying Observable by Iterable at once.

/cc @headinthebox, @samuelgruetter

@cloudbees-pull-request-builder

RxJava-pull-requests #1245 SUCCESS
This pull request looks good

@cloudbees-pull-request-builder

RxJava-pull-requests #1246 SUCCESS
This pull request looks good

lastOption match {
case Some(element) => element
case None => default
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not use Option.getOrElse to implement this?

Copy link
Member Author

Choose a reason for hiding this comment

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

Forgot it... Already updated.

@cloudbees-pull-request-builder

RxJava-pull-requests #1252 SUCCESS
This pull request looks good

@samuelgruetter
Copy link
Contributor

LGTM

@headinthebox
Copy link
Contributor

In this context, why call toList something else? I think we should have both toList and .toSeq.

def toSeq: Observable[Seq[T]] = {
    Observable.jObsOfListToScObsOfSeq(asJavaObservable.toList)
      : Observable[Seq[T]] // SI-7818
  }

@samuelgruetter
Copy link
Contributor

The reason why I changed toList to toSeq is that if RxJava gives us a java.util.List, we want to convert it to something Scala-ish without copying it, and there's no non-copying conversion from java.util.List to scala.collection.immutable.List, but there's a non-copying conversion from java.util.List to scala.collection.mutable.Buffer, which is a subtype of scala.collection.Seq. See JavaConversions for what conversions are possible.

I agree that having a toList method would be good, but if we want to do it efficiently, we should not implement it in terms of the Java toList and convert it to a Scala List by copying, but we should implement toList in Scala, using a scala.collection.mutable.ListBuffer, which provides constant-time append and constant-time toList.

Conflicts:
	language-adaptors/rxjava-scala/src/examples/scala/rx/lang/scala/examples/RxScalaDemo.scala
@cloudbees-pull-request-builder

RxJava-pull-requests #1275 SUCCESS
This pull request looks good

@benjchristensen
Copy link
Member

Is this ready to be merged and released in 0.19.1?

@samuelgruetter
Copy link
Contributor

Is this ready to be merged and released in 0.19.1?

imo yes

@headinthebox
Copy link
Contributor

@samuelgruetter Will you take adding proper.toList? and rename .toList to .toSeq?

@headinthebox
Copy link
Contributor

@benjchristensen Sorry missed this yesterday.

@benjchristensen
Copy link
Member

So ready for 0.19.2 ... or do you want another change @headinthebox ?

@samuelgruetter
Copy link
Contributor

@headinthebox I'm sorry I don't have time right now for toList/toSeq (I'm leaving for Oregon).

@headinthebox
Copy link
Contributor

OK, ill fix it over the weekend.

@zsxwing
Copy link
Member Author

zsxwing commented Jun 15, 2014

@headinthebox, do you have time for this? If not, I can take it.

@headinthebox
Copy link
Contributor

@zsxwing I won't say no to that, go for it!

@cloudbees-pull-request-builder

RxJava-pull-requests #1284 SUCCESS
This pull request looks good

@zsxwing
Copy link
Member Author

zsxwing commented Jun 15, 2014

I added toSeq to BlockingObservable and deprecated toList. I think toList is not necessary since people can always call toList on a Seq. @headinthebox what do you think?

@headinthebox
Copy link
Contributor

Honestly, I actually don't care to much about toBlocking, that is useful for small samples but other than than you should not use it. If you look at Iterable[T], there is a whole bunch of toXXX, and it it is those that I was after with .toList, except that of course in the context of Rx this returns an Observable[List[T]]. So to make a long answer short, I really want def toList: Observable[List[T]] = ???. Note that if we would be pedantic, one could argue its should be be toList() in this case, but that is probably too subtle a distinction.

def to[Col[_]]: Col[A]
def toArray: Array[A]
def toBuffer[B >: A]: Buffer[B]
def toIndexedSeq: immutable.IndexedSeq[A]
def toIterable: Iterable[A]
def toIterator: Iterator[A]
def toList: List[A]
def toMap[T, U]: Map[T, U]
def toParArray: ParArray[T]
def toSeq: Seq[A]
def toSet[B >: A]: immutable.Set[B]
def toStream: immutable.Stream[A]
def toString(): String
def toTraversable: Traversable[A]
de toVector: Vector[A]

@zsxwing
Copy link
Member Author

zsxwing commented Jun 15, 2014

@headinthebox I removed the previous commit about toSeq and toList of BlockingObservable. Since these toXXX methods are about Observable, I think this PR can be merged now. I will open another PR to add these toXXX ones to Observable.

@cloudbees-pull-request-builder

RxJava-pull-requests #1285 SUCCESS
This pull request looks good

benjchristensen added a commit that referenced this pull request Jun 16, 2014
RxScala: Add the rest missing methods to BlockingObservable
@benjchristensen benjchristensen merged commit dcad052 into ReactiveX:master Jun 16, 2014
@zsxwing zsxwing deleted the rxscala-bo branch June 17, 2014 01:47
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

5 participants