Important PR: Complete API work, adding Scala testtools, big refactoring #63

galderz · 2013-10-16T08:48:43Z

No description provided.

* Provide only wrapping functionality around java RecordParser. * Removed old tests * Fixed event bus message typing * Clean up package objects * An execution context for Vert.x to stay on the same thread * Reimplemented SharedData - there are many open questions for this.. * Remove unnecessary Map of handlers * Removed VertxConverters * private[this] trait VertxWrapper to not expose it to users * Provide apply methods to Pump factory

* Fix build script stuff to be able to use TestVerticle * TestVerticle now uses JavaClassRunner and is only available in /test * Class loader fixes * Added http tests and websocket tests * Added fileSystem tests * Added BufferTests for String, Int, Byte, Long, Float, Short, Buffer and Double are complete * Added NetTests * Added chmod tests * Added PumpTests * Refactored buffers * Added event bus tests

* Added test stub for DnsClient API

* Ported DnsExceptions to scala

AsyncResults

* Fixes the the ScalaInterpreterTest * Put VerticleClass used by ScalaInterpreterTest back to its original location. * Restore console output for script vs class interpret stages. This helps the user understand what is going and understand why the console shows some errors when it first tries to run the file as a script and if it fails, it fallsback on running it as a verticle class.

Narigo · 2013-10-16T17:09:56Z

We can close the other PR then, I guess :)

Narigo · 2013-10-17T13:30:17Z

src/main/scala/org/vertx/scala/core/json/Json.scala

+ *
+ * @param elements The elements to put into the JsonArray.
+ * @return A JsonArray containing the provided elements.
+ */
 def arr(fields: Seq[Any]): JsonArray = {
 val a = new JsonArray()
 fields.foreach(f => addToArray(a, f))
 a
 }


I think we should kick the Json.apply() methods and only have Json.obj(fields: (String, Any)) and Json.arr(fields: Any) then. It's a bit more verbose than using Json(...stuff...) but since we always need to put a Seq[Any] in for arrays, that means you always have to do a Json.arr(List(...)) instead of being able to do Json.arr(x1, x2, ...)

Hmmm, I guess you mean Json.arr(fields: Seq[Any]) instead of Json.arr(fields: Any) in the first line?

Argh, sorry, github formatting ate parts of my comment due to asterisks. I want to have these two methods:

def obj(fields: (String, Any)*): JsonObject def arr(fields: Any*): JsonArray

And probably kick out the Json.apply(fields: (String, Any)*) and Json.apply(elements: Seq[Any]) methods as they only add a very small amount of syntactic sugar compared to a much more clear syntax. Actually, Json(List(1, 2, 3)) is even 2 characters more verbose than Json.arr(1, 2, 3), so it's only adding value for creating JsonObjects ;)

Fine by me :)

galderz · 2013-10-17T13:55:11Z

issues.md

@@ -0,0 +1,11 @@
+# EventBus issues


Instead of adding a new file, why not add it to the README file? This way this is shown in the homepage of the github repo...etc.

Actually this wasn't really intended and just for myself to make me not forget this issue. But you're right, we should add it to the readme file as long as we don't have a nice solution for it. Could you do that? Thanks

galderz · 2013-10-17T15:04:51Z

I've not finished yet but gotta leave it here for today. I'll finish the review tomorrow.

Narigo · 2013-10-17T16:09:08Z

Thanks for your time @galderz ! Hope the other contributors can have a look, too. I'd really like to see a release with more methods available. We rely on our own build right now, that we've taken out of these custom branches.

@SaschaSchmidt made some tests for DNS now, but I have to admit I didn't really review them yet: https://github.com/campudus/mod-lang-scala/commit/df3a09b19af12e142f979cfc7e301bd0a4882568

If you'd pull this in, the tests should run through and the whole build should be successful once again! 👍

Narigo · 2013-10-17T22:33:30Z

I saw that Gradle fails the test for ScriptingTest.scala with a FileNotFoundException. That file and the class org.vertx.scala.tests.lang.ScalaScriptingTest came from me as I wanted to see how using Scripts for testing would work. If you want, you can delete it, but maybe it would be better if you'd try to do what I wanted to achieve:

The gradle template has a few integration tests as scripts in different languages. It would be nice to see something like this working for Scala, too. AIUI, the tests for the interpreter just create a ScalaInterpreter and let it interpret these predefined files. That's fine, but we cannot see how the interpreter would pick up a script itself like the template does. Can we somehow fix this, please?

galderz · 2013-10-18T07:01:35Z

src/main/scala/org/vertx/scala/core/VertxExecutionContext.scala

+import scala.concurrent.ExecutionContext
+import org.vertx.scala.core.logging.Logger
+
+trait VertxExecutionContext extends ExecutionContext {


What is this for exactly? I mean, this context class extending scala.concurrent.ExecutionContext, what are you using it for?

You need a ExecutionContext for Futures and Promises. It is used implicitly by the map/flatMap methods in a Future for example. For Vert.x we need to use the "run directly on this thread" concept. If we'd use the global implicit ExecutionContext provided by Scala, Vert.x would break due to the different Threading model in it.

Disclaimer: Tim helped building this class, so I'd expect it to be correct from the logical point of view ;) and my code ran fine with using Futures and this ExecutionContext. :)

* README file is now asciidoc and added content * Added a quickstart * Added a section on current issues * Build status not shown correctly with asciidoc

galderz · 2013-10-20T20:55:13Z

I've fixed ScriptingTest and bundled all minor changes, including readme file work, into a single commit. This is now ready to go in as far as I'm concerned.

galderz · 2013-10-21T16:30:46Z

I temporarily put that while trying to get it to work. Let's try to make it more generic...

galderz · 2013-10-21T16:57:08Z

@Narigo It needs some work to make it generic. I suggest we add as is and I'll make it more generic in the next few days? As it is, all tests pass and the code looks good.

Narigo · 2013-10-21T17:37:50Z

@galderz sure, go ahead! It would be nice to have it on master and send PRs against master again ;)
I think we need to check the Json stuff, too. Json.arr() with variable argument list instead of Seq[Any]. And if we want to release soon, we should consider a few things:

Shall we leave DNS out of it and build a 0.2.0 against Vert.x 2.0.2-final?
Documentation?
Examples?

Narigo · 2013-10-22T04:42:52Z

Why did the Travis build fail for OpenJDK 7? I don't really get its output :(

galderz · 2013-10-22T13:10:25Z

@Narigo Let's integrate this and you can send a pull request for the JSon stuff?

We should not take DNS out now. We're in no rush to get 0.2.0 out, so let's leave it in and release once we have a Vert.x version that includes it.

For docu, I'd add whatever it's needed to the root readme file. Send a PR if there's anything specific you want in there.

Examples should be added to this repo. I'm planning to add a script one and some more, so you can leave that with me.

galderz · 2013-10-22T13:11:34Z

No idea about OpenJDK7 failure.

Narigo · 2013-10-22T20:22:55Z

@galderz I'd really like to have another release with all of that ASAP, actually. I have some dependencies on this branch already and ported some things to it. I wouldn't mind to do some work on it to get DNS out into another branch (for 0.3.0) so we can implement all the things that come with Vert.x 2.1.0 on it. Otherwise we'd have to implement all those new things, too, in order to have the complete Vert.x API available. That means at least HTTP compression, UDP, EventBus timeouts and I guess even more things.

So to recap my proposal:

Merge this into master @galderz
Polish this up for a code release, i.e. change Json API as we discussed earlier in this PR. @Narigo
Put all the DNS related things into a new branch that targets Vert.x 2.1.0 @Narigo
Fix the Travis build (investigate issue about OpenJDK7) - can you do that @galderz ?
Release the module as io.vertx~~mod-lang-scala~~0.2.0 @galderz
Add documentation and examples - maybe we can get some help here?

If you're okay with that, please start by merging this PR or send me a note to do so. Then I can start working on it right away. Maybe @SaschaSchmidt can help us with docs and examples a little bit this week, if we can merge it ASAP. ;)

galderz · 2013-10-23T08:35:17Z

All looks good to me. The only thing is that we don't need a separate branch for the DNS + Vertx 2.1 stuff. Once the release is out, which should be in the next few days, master can be used for that. You can always have a topic branch with this work.

galderz · 2013-10-23T08:45:40Z

The OpenJDK7 stuff looks good now. It might a glitch or something, but eventually we'll be focusing on the results of cloudbees, once we've got time to beef that up.

This is integrated now. @Narigo you can now send the PRs for the JSON and pull out the DNS stuff.

Narigo and others added 6 commits October 16, 2013 09:04

Added DnsClient API

0ba6760

* Added test stub for DnsClient API

Update to vert.x 2.0.3-SNAPSHOT

985735e

* Ported DnsExceptions to scala

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

1cacb4e

AsyncResults

Narigo reviewed Oct 17, 2013
View reviewed changes

galderz mentioned this pull request Oct 17, 2013

Scala API - please review #51

Closed

galderz reviewed Oct 17, 2013
View reviewed changes

galderz reviewed Oct 18, 2013
View reviewed changes

This was referenced Oct 18, 2013

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

Closed

Rebase with latest Vertx Gradle template files #56

Closed

vertx timer() function should return timer id #55

Closed

SaschaSchmidt and others added 2 commits October 18, 2013 17:46

DnsTests complete

e2ccded

Fix ScriptingTest by loading script from classloader

263aac7

* README file is now asciidoc and added content * Added a quickstart * Added a section on current issues * Build status not shown correctly with asciidoc

galderz closed this Oct 23, 2013

galderz referenced this pull request Nov 8, 2013

remove Json.apply() methods and simplify Json.arr() usage

10f8fa9

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Important PR: Complete API work, adding Scala testtools, big refactoring #63

Important PR: Complete API work, adding Scala testtools, big refactoring #63

galderz commented Oct 16, 2013

Narigo commented Oct 16, 2013

Narigo Oct 17, 2013

galderz Oct 17, 2013

Narigo Oct 18, 2013

galderz Oct 18, 2013

galderz Oct 17, 2013

Narigo Oct 18, 2013

galderz commented Oct 17, 2013

Narigo commented Oct 17, 2013

Narigo commented Oct 17, 2013

galderz Oct 18, 2013

Narigo Oct 18, 2013

galderz commented Oct 20, 2013

galderz commented Oct 21, 2013

galderz commented Oct 21, 2013

Narigo commented Oct 21, 2013

Narigo commented Oct 22, 2013

galderz commented Oct 22, 2013

galderz commented Oct 22, 2013

Narigo commented Oct 22, 2013

galderz commented Oct 23, 2013

galderz commented Oct 23, 2013

Important PR: Complete API work, adding Scala testtools, big refactoring #63

Important PR: Complete API work, adding Scala testtools, big refactoring #63

Conversation

galderz commented Oct 16, 2013

Narigo commented Oct 16, 2013

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

galderz commented Oct 17, 2013

Narigo commented Oct 17, 2013

Narigo commented Oct 17, 2013

Choose a reason for hiding this comment

Choose a reason for hiding this comment

galderz commented Oct 20, 2013

galderz commented Oct 21, 2013

galderz commented Oct 21, 2013

Narigo commented Oct 21, 2013

Narigo commented Oct 22, 2013

galderz commented Oct 22, 2013

galderz commented Oct 22, 2013

Narigo commented Oct 22, 2013

galderz commented Oct 23, 2013

galderz commented Oct 23, 2013