add tests for validation

[jvkassi]

Hello jaumard,

Here is my push

[jvkassi]

fix #28

[jaumard]

Great job here ! Almost perfect ! :D I think a lot of code is duplicate here. I think all @QUERY should extends @Route annotation because they all do the same (only the name is changing).
Like this all get targets() disappear, get path() need to stay because of the __filname and the handler(app, annotation) can simply pass the data to @Route like handler(app, annotation, method) who need to be create. Do you see what I mean ? Like this no more duplicate code :)

[jaumard]

Also for tests @POST/PUT/PATCH need an object with validation ^^ it make more sense to have data validation for this kind of routes.

[jaumard]

Hum one more thing ^^ the Readme could be updated to show @Query possibility or user will not know it's possible :D

[jvkassi]

@jaumard looking better ?

[jaumard]

Yeah much better :) one question... Why did you create the Query class ? Because now Route and Query do the same things. I was thinking of using Route as master class directly. Or I miss something just want to know if there a good reason :)

[jvkassi]

I needed the query class for doing something more specific like adding the validate object into the config object for the route definition. It seems even more generic like this the post,get, options class are much more lightweight

[jaumard]

Ho !!! I see !! I miss it but you don't need this in my opinion the annotation should look like at the end :

/**
 * Return some info about this application
 * @GET(path:{'/default/info'}, config: {validate: {
 * query: { infos: Joi.sring().required() }
 * }})
 */

All the object you put in the annotation should reflect the Hapi API too ^^ this way you don't need the Query class and only Route is needed. This way Route and all other annotations have the same way to define a route. More coherent. Sorry didn't see it before my bad

[jaumard]

I want this because validate is fine but I add CORS support options too into express so your code actually skip every other routes options except validate ^^

[jvkassi]

yeah, was thinking about it like a shortcut. ok updating

[jaumard]

Sorry about this :) yeah shortcut will be find if validate was the only option available but here there too many. So now can you remove the Query class to use Route ? And we are ready to merge/deploy :D

[jvkassi]

hmm... sorry i don't understand why i have to remove the query class. It's a good layer, i think. Can you review again or explain to me please ?

[jaumard]

Now Query and Route do exactly the same ^^ so why don't use only Route the only difference is this.query witch can be done like :

handler(app, annotation) {
    let infos = {}

    if (!annotation.className) {
      annotation.className = _.last(annotation.filePath.split('/')).replace('.js', '')
    }

    if (_.isObject(annotation.value)) {
      infos = annotation.value
      infos.handler = annotation.className + '.' + annotation.target
      if(this.query) infos.handler = this.query
    }
    else {
      const parts = annotation.value.split(' ')
      infos = {
        method: this.query || parts[0],
        path: this.query ? parts[0] : parts[1],
        handler: annotation.className + '.' + annotation.target
      }
    }

    app.config.routes.push(infos)
  }

[jvkassi]

XD clever

[jaumard]

Thanks ! :D I'll publish it now on npm

[jvkassi]

^^ let's keep up !