Added startFuture and stopFuture methods to ScalaVerticle and improve… #24
Conversation
| @@ -51,14 +52,27 @@ abstract class ScalaVerticle { | |||
| } | |||
|
|
|||
| /** | |||
| * Start the verticle. | |||
| */ | |||
| def start(): Unit = { | |||
There was a problem hiding this comment.
It should be noted that if the user overrides both this method and the startFuture method, this here will only be called if the overridden method startFuture calls start. Is this a really good idea?
There was a problem hiding this comment.
That's the same behavior as in AbstractVerticle in the Vertx-core.
vertx-lang-scala/README.md
Outdated
| case Failure(t) => promise.failure(t) | ||
| } | ||
|
|
||
| promise.future |
There was a problem hiding this comment.
Why is fSeq not returned directly? You shouldn't need to wrap a promise around it?
This should be enough:
override def startFuture(): Future[Void] = Future.sequence(Seq(
vertx.eventBus().consumer[String]("asd").handler(a => println(a)).completionFuture(),
vertx.eventBus().consumer[String]("asd2").handler(a => println(a)).completionFuture()
))OTOH, why Future[Void] as return value of startFuture? It's Future[Unit] if I read the other changes correctly.
There was a problem hiding this comment.
Yes, you are right. The Promise is a left over from what was there befor.
I removed it and also changed the signature.
| @@ -16,10 +16,10 @@ class ScalaVerticleTest extends FlatSpec with Matchers { | |||
| val cl = new CountDownLatch(1) | |||
| val vertx = Vertx.vertx | |||
| implicit val exec = VertxExecutionContext(vertx.getOrCreateContext()) | |||
| vertx.deployVerticleFuture(s"scala:${classOf[TestVerticle].getName}").foreach(r => cl.countDown()) | |||
| vertx.deployVerticleFuture(nameForVerticle[TestVerticle]).onComplete(t => cl.countDown()) | |||
There was a problem hiding this comment.
The nameForVerticle[XXX] method could actually have its own test :)
There was a problem hiding this comment.
Added a Test :)
| @@ -10,7 +10,7 @@ import scala.util.{Failure, Success} | |||
| */ | |||
| class TestVerticle extends ScalaVerticle{ | |||
|
|
|||
| override def start(): Future[Unit] = { | |||
| override def startFuture(): Future[Unit] = { | |||
| Future.successful(()) | |||
There was a problem hiding this comment.
Umm... This line does nothing?
There was a problem hiding this comment.
Fixed the whole test, including the comments below.
There was a problem hiding this comment.
Changed the whole thing.
| * Stop the verticle.<p> | ||
| * This is called by Vert.x when the verticle instance is un-deployed. Don't call it yourself.<p> | ||
| * If your verticle does things in it's shut-down which take some time then you can override this method | ||
| * and complete the futrue some time later when clean-up is complete. | ||
| * | ||
| * @return a future which should be completed when verticle clean-up is complete. | ||
| */ | ||
| def stop(): concurrent.Future[Unit] = { | ||
| def stopFuture(): concurrent.Future[Unit] = { | ||
| stop() |
There was a problem hiding this comment.
If stop() throws, this won't return you a failed Future, but a thrown exception. It would be nicer to do this:
def stopFuture(): concurrent.Future[Unit] = Future.fromTry(util.Try(stop()))If you want to let the ExecutionContext decide when to run it, you could do this:
def stopFuture(): concurrent.Future[Unit] = Future(stop())This should also capture a thrown exception into a failed Future. Same applies to startFuture().
There was a problem hiding this comment.
Uhh, good catch! I picked the 2nd option.
| @@ -24,7 +24,7 @@ class VertxExecutionContextTest extends FlatSpec with Matchers { | |||
|
|
|||
| class PromiseTestVerticle extends ScalaVerticle { | |||
|
|
|||
| override def start(): Future[Unit] = { | |||
| override def startFuture(): Future[Unit] = { | |||
There was a problem hiding this comment.
No need to wrap a promise around the future. Is that what you want to test or just the execution context? Then it should suffice to have res.map(_ => ()) at the end and remove all lines with promise in this method.
There was a problem hiding this comment.
Not sure if there is a test for overriding start (and stop) with a success and failure case, just to check if the usual startFuture and stopFuture end in a success or failure respectively. Do you have a test for stopFuture? If not, might make sense to add something like you did for startFuture as well.
|
Looks good to me! 👍 |
Added startFuture and stopFuture methods to ScalaVerticle and improve…
…d docs