Skip to content

Commit

Permalink
set up stubs for Scala Observable and its testing
Browse files Browse the repository at this point in the history
  • Loading branch information
samuelgruetter committed Sep 6, 2013
1 parent 9cef1bd commit c159625
Show file tree
Hide file tree
Showing 2 changed files with 36 additions and 1 deletion.
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
package rx.lang.scala

class Observable[+T](val asJava: rx.Observable[_ <: T]) extends AnyVal {

}

object Observable {
import scala.collection.JavaConverters._
import rx.{Observable => JObservable}

def apply[T](args: T*): Observable[T] = {

This comment has been minimized.

Copy link
@retronym

retronym Oct 7, 2013

@samuelgruetter This method is now overloaded with others, which robs all of the others of any type safety. You should rename this one to fromXxx or something.

scala> object O { def apply[T](ts: T*) = (); def apply(f: (String => Int)) = () }
defined object O

scala> O((i: String) => i.toString.size.toString)

This comment has been minimized.

Copy link
@samuelgruetter

samuelgruetter Oct 11, 2013

Author Contributor

There's a difference between your object O and the Scala Observable: The Scala Observable has a type parameter which depends on the argument(s) given to apply. So if you call Observable(badFunction), you get an Observable[FunctionType] instead of an Observable[ExpectedType], and you will notice that something is wrong, because you can't use your observable the way you want.

So currently I think that there is no real-world example where renaming apply(T*) to fromXxx(T*) helps detect a programmer's mistake at compile time. But if someone finds such an example, I'd be in favor of doing the suggested renaming.

This comment has been minimized.

Copy link
@retronym

retronym Oct 11, 2013

Erik Meijer was trying to use this API and failed. He's pretty new to Scala and doubted the compiler:

> On Sun, Oct 6, 2013 at 4:01 PM, Erik Meijer <erik.meijer@meijcrosoft.com> wrote:
OK, the Scala type checker does not complain about the highlighted value; which apparently is not of type Subscription.

val clickStream = Observable((observer: Observer[AbstractButton]) => {
       val onClicked: Reaction = { case ButtonClicked(b) => { observer.onNext(b) } }
       reactions += onClicked
       ()=>{ reactions -= onClicked }
     })

http://stackoverflow.com/questions/2510108/why-avoid-method-overloading/2512001#2512001

This comment has been minimized.

Copy link
@retronym

retronym Oct 11, 2013

If you want to keep overloading, I would suggest accepting a Seq[A] rather than an A*. That cuts out a lot of potential errors, at the small cost of the user giving up varargs syntax.

This comment has been minimized.

Copy link
@samuelgruetter

samuelgruetter Oct 11, 2013

Author Contributor

I think that would be a good idea, and I could live with Observable(Seq(1, 2, 3)) instead of Observable(1, 2, 3).
@vjovanov since we also discussed this, what do you think?

This comment has been minimized.

Copy link
@vjovanov

vjovanov Oct 11, 2013

Renaming a method to fromXXX is as verbose as Seq() so I would give up varargs altogether.

This comment has been minimized.

Copy link
@samuelgruetter

samuelgruetter Oct 11, 2013

Author Contributor

I'm now replacing apply(T*) by apply(Seq[T]). But now how do we want to create an empty observable:

  • Observable(): as before, requires adding a no-arg apply, not consistant with Observable(Seq(...))
  • Observable(Seq()): slightly clumsy
  • Observable.empty: adding the 192nd method...

This comment has been minimized.

Copy link
@retronym

retronym Oct 11, 2013

What is the expected frequency of calling the various overloads? Ideally you the most commonly used one and give that apply, and use different names for the others.

This comment has been minimized.

Copy link
@vjovanov

vjovanov Oct 11, 2013

I think we should be consistent with Scala collections. Maybe, we should add empty as the 192nd method :)

This comment has been minimized.

Copy link
@samuelgruetter

samuelgruetter Oct 11, 2013

Author Contributor

I replaced apply(T*) with apply(Seq[T]), but I'm not very happy with it. See here how clumsy it can be.

In order to be consistent with Scala collections, we should have a varargs apply and empty method, and all other constructors should have names different from apply, but this would not really solve the original problem.

Here's a list of all apply overloads we currently have:

  • def apply[T](asJava: rx.Observable[_ <: T])
  • def apply[T](func: Observer[T] => Subscription), in Java: Observable.create
  • def apply[T](exception: Throwable), in Java: Observable.error
  • def apply[T](args: T*), or def apply[T](args: Seq[T]), in Java: Observable.from
  • def apply(range: Range), in Java: Observable.from
  • maybe added later: constructor for Scala Future -> Observable conversion

and it's difficult to estimate their usage frequency, except that apply(Throwable) is probably not very often used.

This comment has been minimized.

Copy link
@retronym

retronym Oct 11, 2013

Another meta-point: stick to the names in the Java API unless you have a clear improvement on them.

I would suggest: asScala or (fromJava), create, error, apply, and from.

This comment has been minimized.

Copy link
@samuelgruetter

samuelgruetter Oct 11, 2013

Author Contributor

I'd be ok with this, but I'm not yet sure whether we want apply(Seq[T]) or apply(T*).

  • apply(T*), together with empty would be like Scala's List, but it would not really solve Erik's problem: His snippet would pass, but clickStream would be of type Observable[Observer[AbstractButton] => () => Unit].
  • apply(Seq[T]) would solve Erik's problem, but it's clumsy

This comment has been minimized.

Copy link
@retronym

retronym Oct 11, 2013

The only reason he tried to call apply() was that we was looking at the other overload. If that had a different name, he wouldn't have called apply.

This comment has been minimized.

Copy link
@vjovanov

vjovanov Oct 11, 2013

Comments from the meeting today:

  • def apply[T](func: Observer[T] => Subscription) must work.
  • Others are nice to have as overloads.
  • Unlike with collections def apply[T](args: T*) is not very important. It is mostly used in tests.
new Observable(JObservable.from(args.toIterable.asJava))
}
}

import org.scalatest.junit.JUnitSuite

class UnitTestSuite extends JUnitSuite {

import org.junit.{ Before, Test }
import org.junit.Assert._
import org.mockito.Matchers.any
import org.mockito.Mockito._
import org.mockito.{ MockitoAnnotations, Mock }
import rx.{ Notification, Observer, Subscription }
import rx.observables.GroupedObservable
import collection.JavaConverters._

@Test def testTest() = {
println("testTest()")
assertEquals(4, Observable(1, 2, 3, 4).asJava.toBlockingObservable().last())
}


}
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ package rx.lang.scala

import org.scalatest.junit.JUnitSuite

class UnitTestSuite extends JUnitSuite {
class OldUnitTestSuite extends JUnitSuite {
import rx.lang.scala.RxImplicits._

import org.junit.{ Before, Test }
Expand Down

0 comments on commit c159625

Please sign in to comment.