use request logger whenever it's available

[osher]

I'm still not sure about line 43.

I did not touch it - but I feel there's a problem there - for the least of it, it should be next(err) or maybe even next(err2)...

[coveralls]

Coverage increased (+0.02%) to 96.333% when pulling de729d4aac4e89d82d5d151a942b03f447476427 on osher:patch-1 into 07a706f461833ca1642981a61c8ce72e6a8a260b on theganyo:master.

[theganyo]

This is interesting, but I haven't seen putting a "log" attribute on request & response as a convention. Is this commonly used?

[osher]

yes. Restify does it for sure, I saw express users do it (although it's not a part of the common infra that comes with express), I'm not sure about hapi - but I'm quite positive there is a plugin that does.

I'm on a work station now, I'll get it working shortly

[osher]

um.. - I need a point in the right direction regarding the last stage of error handling.

If I get it right:

• calling next(err) resorts to the default error handling of the used framework, which is usually a rude 500 page with HTML content.
• the handler is there to ensure the resopnse is sent as application/json Therefore
• the handler should call next(null, formattedJSONBody)

so I adjust my implementation following that understanding, I'd appreciate if you can confirm I'm right.

Also - we could use fittingDef to customize the output more - I'd love any inputs on that...

[coveralls]

Coverage increased (+0.04%) to 96.352% when pulling d64172dbebf235acb59f3f5eaf9cce7a9da6c778 on osher:patch-1 into 07a706f461833ca1642981a61c8ce72e6a8a260b on theganyo:master.

[osher]

re. how common that is: restify - out of the box (http://restify.com/#log) hapijs - out of the box (http://hapijs.com/tutorials/logging) express - middleware that provides you with req.log (https://www.npmjs.com/package/express-bunyan-logger, and https://www.npmjs.com/package/express-pino-logger, - which I only found about today, when I worked with express we did it ourselves, it's just a single little function that just req.log = logger.createLogger(...); next() )

[theganyo]

Fantastic. Yes, I agree with your design. This looks like a great patch. Thanks for your work on it!

Are you finished or do you want to add anything else? (You mentioned configuration above... not sure what you might have in mind.)

[osher]

If you like it as it is and don't see any reason to provide output customization - then lets go.

For honesty's sake - I found one more frequent habit I do not cover which might be common among express users:

req.app.log.

Would you like to support it? if yes - can it be in a later PR?

[coveralls]

Coverage increased (+0.04%) to 96.352% when pulling 9a6f834b6f6a981f9c92d6c95a775163d6ede93f on osher:patch-1 into 07a706f461833ca1642981a61c8ce72e6a8a260b on theganyo:master.

[osher]

Ok. done. I did it too. I also arranged the order of checks by what I believe to be the most commonly used.

I would also update the README.md, but it looks you're not documenting there anything... where should I doc it? I see that the WIKI is empty too :open_mouth:

[osher]

mm. one more thing

If you're aware on the performance penalty of just entering a try block - then you may want to consider starting the try at line 39 (or even try{} only the JSON.stringify).

let me know what you think

[theganyo]

Documentation had been generally left to the swagger project that uses this module. (Although that probably needs to change as that project is no longer in sync with what's happening here.) That said, I've been documenting changes in the release notes.

Re: Optimization, yes, we could move the exit conditions (lines 27-38) to before the try block. Also, it appears that you could move your log assignment down to the catch block to ensure it's only defined when used.

[osher]

So wierd. It fails test. We might have uncovered something else here, but this should belong to another issue. The problematic line is the actual call to next(err) in

   if (context.statusCode === 500 && !fittingDef.handle500Errors) { return next(err); }

I added a //TODO comment and left it in the try{} block

[coveralls]

Coverage increased (+0.04%) to 96.352% when pulling 544108065fa88f32c785fb8d29c8926770ccf831 on osher:patch-1 into 07a706f461833ca1642981a61c8ce72e6a8a260b on theganyo:master.

[coveralls]

Coverage increased (+0.04%) to 96.352% when pulling 73f7557db0e0e4311b7357a0fef2c32d6733b73b on osher:patch-1 into 07a706f461833ca1642981a61c8ce72e6a8a260b on theganyo:master.

[theganyo]

Thanks!

[osher]

Ah! great!!! thanks, super :smile: