Scala API - please review #51

Narigo · 2013-09-18T13:53:33Z

Please do not integrate, just review and send me notes about it :)

It should have the complete Java API now, but there are some things
about it:

The event bus is hard to do as handlers which get implicitly
converted create new objects. That means that these new objects won't be
able to be unregistered later. I've added two methods that should be
able to deal with it, but maybe you have a better idea how to handle this.
I've copy & pasted most of the JavaDoc into the Scala stuff. I guess
there'll be some comments that don't work out very well.
I did not implement any(!) tests and actually deleted what was there
before. Mostly because there is no testtools for Scala yet and using the
Java API to test the Scala API is not really nice to do. Is someone
working on testtools already? Would be very nice to use it then and test
everything with it.

As I've done quite a bit of work now during the nights, I'd really like
to see other people working on it - I'd be really happy if someone else
would be able to implement the tests and fix what's wrong with the code.

There are a lot of methods like this, to have a fluent API:

class SomeWrappedJavaClass {
  def doStuff(someParameter: String, doneHandler: String => Unit): SomeWrappedJavaClass = ...
}

I'm not sure what would be better: The one posted above or def doStuff(someParameter: String)(doneHandler: String => Unit)= ... to be
able to call it like this: doStuff("hello") { answer => println("got a reply: " + answer) }

RouteMatcher - I'd like to kick this out of the API completely. As a
replacement we could add a real idiomatic RouteMatcher, which might look
like this:

class MyRequestHandler extends RouteMatcherTrait {
  def requestHandler(request: HttpServerRequest) = {
    case Get("/") => SendFile("index.html")
    case Post("/login") =>
      if (checkLoginPostData(request)) {
        SendFile("logged_in.html")
      } else {
        SendFile("error.html")
      }
  }
}

Using Futures and Promises, maybe we could use one of them as a
replacement for AsyncResult...? It would also be extremely nice to have
them as a return value of functions, so working with the file system
could look like this:

for {
  firstFile <- fs.open("some_file.txt")
  secondFile <- fs.open("some_other_file.txt")
  firstText <- fs.read(firstFile)
  secondText <- fs.read(secondFile)
} yield {
  firstText + secondText
}

That's not a good example, but think about "scripting" in this style -
open file, read it, transform it, write the output somewhere else.

Conflicts: src/main/scala/org/vertx/scala/core/sockjs/SockJSSocket.scala

galderz · 2013-09-18T14:06:06Z

@Narigo Looking into this, looks very good at first glance :)

I'll work on the Scala testtools stuff once I've finished getting Scala scripts to run as well as Scala classes.

galderz · 2013-09-18T14:13:15Z

src/main/scala/org/vertx/scala/VertxWrapper.scala

+   *
+   * @return The internal type instance.
+   */
+  def toJava(): InternalType = internal


Should we make this package private for org.vertx.scala? This way we'd avoid exposing it to end users.

Not sure about this, actually. We could use it whenever the Java API adds new functionality and needs some Java type...

I think it's more important users don't get somehow attached to the Java API. I'd rather keep it private and if people complain, maybe open it up. The opposite move, having it public and then wanting to make it private it's more problematic. IOW, if it doubt, leave it out. Let's expose as little as possible that gets the job done.

Plus, even if the Java API adds new functionality, we probably wanna wrap it somehow and we probably don't want people to use it as is in the Java world.

As discussed in IRC, I'd private[scala] trait VertxWrapper and let def toJava() stay public. This way developers can get the internal Java value, if they need it for other APIs (like includeable modules).

galderz · 2013-09-18T15:00:48Z

src/main/scala/org/vertx/scala/core/streams/Pump.scala

+  /**
+   * Create a new {@code Pump} with the given {@code ReadStream} and {@code WriteStream}
+   */
+  def createPump[A <: ReadStream, B <: WriteStream](rs: ReadStream, ws: WriteStream) = {


Why not name these createPump methods apply?

To be consistent with the Java API - I guess that would make it easier for devs coming from other languages to use it. Shall we provide both?

+1 for both

galderz · 2013-09-18T15:03:40Z

Sorry guys, closed by mistake :)

Narigo · 2013-10-01T15:16:18Z

Here is another batch of additions for the Scala API. We've fixed various bugs and added a lot of tests (thanks @SaschaSchmidt !). Before you ask: Yes, he has signed the Eclipse CLA ;)

After we fixed the classloading issues with @purplefox , we have trouble doing the scripting tests. I've added another script to run like the integration_tests scripts from the gradle template, but that didn't work either. Please someone have a look - I have no clue what we'd have to change to get them running again.

Thanks for reviewing!

Narigo · 2013-10-02T10:49:25Z

@raniejade println is used only in the ScalaInterpreter right now. We can kick them out as soon as it works... Could you have a look into it? The only failing tests are for the ScalaInterpreter right now and if they are ok, we should be able to merge this quite soon.

raniejade · 2013-10-02T23:31:22Z

Sure thing, I'll check on it right away after I have the time.

On Wed, Oct 2, 2013 at 8:49 PM, Joern Bernhardt notifications@github.comwrote:

@raniejade https://github.com/raniejade println is used only in the
ScalaInterpreter right now. We can kick them out as soon as it works...
Could you have a look into it? The only failing tests are for the
ScalaInterpreter right now and if they are ok, we should be able to merge
this quite soon.

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/51#issuecomment-25529738
.

Ranie Jade Ramiso
Software Engineer | Open Source Enthusiast | Professional Slacker
Philippines

w: raniejaderamiso.com
e: raniejaderamiso@gmail.com

Narigo · 2013-10-14T22:23:58Z

Did you manage to have a look at the interpreter stuff @raniejade ?

raniejade · 2013-10-14T22:29:28Z

No, sorry my I'm currently out of the country (business trip, sigh). No
laptop too :(, probably gonna buy one next week :D.

On Tue, Oct 15, 2013 at 9:24 AM, Joern Bernhardt
notifications@github.comwrote:

Did you manage to have a look at the interpreter stuff @raniejadehttps://github.com/raniejade?

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/51#issuecomment-26293977
.

Ranie Jade Ramiso
Software Engineer | Open Source Enthusiast | Professional Slacker
Philippines

w: raniejaderamiso.com
e: raniejaderamiso@gmail.com

i moved it out before we forget it as i guess this might cause trouble later...

Narigo · 2013-10-15T00:43:41Z

tests are still missing, but DnsClient is included now. Next up: Datagram stuff...

AsyncResults

galderz · 2013-10-17T13:37:10Z

Closing. I've fixed the ScalaInterpreterTest and squashed commits a bit for easier management in #63

galderz · 2013-10-17T13:37:34Z

Everyone, comment on #63 ASAP :)

galderz · 2013-10-17T14:21:46Z

FYI @raniejade no need to look at the interpreter stuff any more :)

Narigo and others added 19 commits August 30, 2013 00:03

quite some refactoring...

07b4b64

a whole lot of refactoring to get complete java api...

d2caeef

Provide only wrapping functionality around java RecordParser.

aa021bb

kick old tests

67e227c

Buffer.append()

f8ec3a6

not working event bus stuff

67373f1

Merge remote-tracking branch 'official/master' into complete-api

26e293e

Conflicts: src/main/scala/org/vertx/scala/core/sockjs/SockJSSocket.scala

event bus message typing

912397c

kicked unused conversion (Message[X] => Unit to Handler[Message[X]])

630d7fc

register / unregister handler (locally might work)

0b4fc29

clean up package objects

5653788

forgot body in message... :)

ad75c30

refactoring

11d264d

An execution context for Vert.x to stay on the same thread

ad62c5b

reimplemented SharedData - there are many open questions for this..

61121b6

HttpClient still missing and everything above core.http package to check

b65085d

completed API and started switching to idiomatic scala

58defbf

a small issue regarding the event bus :(

ef5aa5a

that's a lot of stuff. please someone write testtools and tests!

d4eec90

galderz reviewed Sep 18, 2013
View reviewed changes

Remove unnecessary Map of handlers

622faa2

galderz reviewed Sep 18, 2013
View reviewed changes

galderz closed this Sep 18, 2013

galderz reopened this Sep 18, 2013

SaschaSchmidt and others added 10 commits September 30, 2013 16:32

PumpTests starts and FileTest chmod test complete

75c568e

buffer refactorings and kicked out unused proprietary vert.x Future

00903a9

pump tests

01da37e

feature warnings

dc4a4ff

Trying to get scripts/interpreting to run again

9def7b5

consistency for sending Int as primitive type

b1c56b2

kicked println

c49ea1c

scala SharedData added (no tests yet)

0c5efa2

check for buffer length not toString

703c4c1

added event bus tests

8224845

This was referenced Oct 4, 2013

StackOverflowError when using vertx.runOnLoop() #58

Closed

HttpServerRequest headers names are not case insensitive #57

Closed

Narigo mentioned this pull request Oct 14, 2013

Add DNS support #59

Closed

Narigo added 4 commits October 15, 2013 01:11

not sure if needed .=scala for interpreter test.

089c6e2

i moved it out before we forget it as i guess this might cause trouble later...

Added DnsClient API

4fb8b1b

added test stub for DnsClient API

23b0f10

use vert.x 2.0.3-SNAPSHOT

16b294c

Narigo added 2 commits October 15, 2013 11:11

ported DnsExceptions to scala

35a72b6

Added Try to AsyncResult implicit, to enable users to pattern match

906e954

AsyncResults

Narigo mentioned this pull request Oct 15, 2013

Use Try[X] instead of AsyncResult[X] #62

Closed

galderz closed this Oct 17, 2013

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Scala API - please review #51

Scala API - please review #51

Narigo commented Sep 18, 2013

galderz commented Sep 18, 2013

galderz Sep 18, 2013

Narigo Sep 18, 2013

galderz Sep 18, 2013

Narigo Sep 18, 2013

galderz Sep 18, 2013

Narigo Sep 18, 2013

galderz Sep 18, 2013

galderz commented Sep 18, 2013

Narigo commented Oct 1, 2013

Narigo commented Oct 2, 2013

raniejade commented Oct 2, 2013

Narigo commented Oct 14, 2013

raniejade commented Oct 14, 2013

Narigo commented Oct 15, 2013

galderz commented Oct 17, 2013

galderz commented Oct 17, 2013

galderz commented Oct 17, 2013

Scala API - please review #51

Scala API - please review #51

Conversation

Narigo commented Sep 18, 2013

galderz commented Sep 18, 2013

galderz Sep 18, 2013

Choose a reason for hiding this comment

Narigo Sep 18, 2013

Choose a reason for hiding this comment

galderz Sep 18, 2013

Choose a reason for hiding this comment

Narigo Sep 18, 2013

Choose a reason for hiding this comment

galderz Sep 18, 2013

Choose a reason for hiding this comment

Narigo Sep 18, 2013

Choose a reason for hiding this comment

galderz Sep 18, 2013

Choose a reason for hiding this comment

galderz commented Sep 18, 2013

Narigo commented Oct 1, 2013

Narigo commented Oct 2, 2013

raniejade commented Oct 2, 2013

Narigo commented Oct 14, 2013

raniejade commented Oct 14, 2013

Narigo commented Oct 15, 2013

galderz commented Oct 17, 2013

galderz commented Oct 17, 2013

galderz commented Oct 17, 2013