search

vert-x / mod-lang-scala

Vert.x 2.x is deprecated - use instead
https://github.com/vert-x3/vertx-lang-scala
Apache License 2.0
77 stars 29 forks source link

Added noMatch route matcher. #185

Closed mgenereu closed 9 years ago

mgenereu commented 10 years ago

noMatch was missing in Scala RouterMatcher wrapper for the Java RouteMatcher. The documentation already contained instructions on how to use this command.

http://vertx.io/core_manual_scala.html#handling-requests-where-nothing-matches

mgenereu commented 9 years ago

The build is working on my end. Is this feature okay?

galderz commented 9 years ago

Yeah, I think the feature is fine, but it'd need to be part of Vert.x Scala 1.1.x. Can you please send a pull request for it?

mgenereu commented 9 years ago

So you want it against something other than master?

mgenereu commented 9 years ago

Closing for now until you comment which branch. Thanks!

galderz commented 9 years ago

@mgenereu Apologies for the delay getting back on this. I was trying to suggest that you send a PR for master, as opposed to sending a PR for branch 1.0.x, which is the other only existing branch. Looking forward to your PR! :smile:

mgenereu commented 9 years ago

This appears to be against master already. I've rebased it again to the latest master.

galderz commented 9 years ago

Yeah, @Narigo added it in master in issue #178. I'll close it.

By the way, the issue it's fixed in a 1.1.0-M1 release we did last year. It was mostly done to cover this particular case. Apologies for forgetting to update this issue.

mgenereu commented 9 years ago

No worries. Thank you!

Narigo commented 9 years ago

@galderz I don't think so, actually. The RouteMatcher is not the same as the Router that we supply in Scala...

galderz commented 9 years ago

@Narigo is right, I was getting confused with something else. noMatch is still not there.

mgenereu commented 9 years ago

So, mergeable?

Narigo commented 9 years ago

The latest build failed - looks like that "Loosen requirement to latest Vert.x 2.1.x" commit doesn't work well with CI?

It would also be a good idea to always add a test for a new feature...

mgenereu commented 9 years ago

Removed that commit. Was just a courtesy as Vert.x is continuing ahead with new versions.

Narigo commented 9 years ago

Maybe @galderz can take a look. I'm unsure about that CI problem as I don't get why / what is broken. Maybe it wasn't even that commit - sorry...

mgenereu commented 9 years ago

It wasn't the commit but it was adding noise to the situation. I can't find where the Router Manager gets tested.