Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Initial draft of version 3 #194

Merged
merged 6 commits into from Sep 25, 2014
Merged

Initial draft of version 3 #194

merged 6 commits into from Sep 25, 2014

Conversation

@arb
Copy link
Contributor

arb commented Sep 17, 2014

First draft of version 3.

Fixes #176
Fixes #177
Fixes #152

@geek geek added this to the 3.0.0 milestone Sep 18, 2014
};

exports.register.attributes = {

pkg: require('../package.json')
};

exports.GoodConsole = require('./reporter');

This comment has been minimized.

Copy link
@geek

geek Sep 18, 2014

Member

Please move these required statements to the top or add a note that they will be required.

Schema.assert('monitorOptions', this.settings);
Schema.assert('monitorSubscribers', this.settings.subscribers);
options = options || {};
var _reporters = options.reporters;

This comment has been minimized.

Copy link
@geek

geek Sep 18, 2014

Member

reporters no need to indicate its protected within the same function

appVer: internals.appVer,
timestamp: Date.now(),
events: self._eventsFilter(self._subscriberTags[uri], subscriberQueue)
if (!err) {

This comment has been minimized.

Copy link
@geek

geek Sep 18, 2014

Member

what do you do if there is an error?


var self = this;
Async.each(this._reporters, function (item, next) {

This comment has been minimized.

Copy link
@geek

This comment has been minimized.

Copy link
@arb

arb Sep 18, 2014

Author Contributor

Says in the docs that it's for hapi and to continue to use Async in your own code. If you really want me to switch though I can.

var subscriberQueue = self._subscriberQueues.http[uri];
if (!subscriberQueue.length) {
return;
if (error) {

This comment has been minimized.

Copy link
@geek

geek Sep 18, 2014

Member

err or error, I see it both ways in this file

This comment has been minimized.

Copy link
@arb

arb Sep 18, 2014

Author Contributor

Should be error everywhere now.

if (!subscriberQueue.length) {
return;
if (error) {
return callback(new Error('Error starting _reporters ' + error.toString()));

This comment has been minimized.

Copy link
@geek

geek Sep 18, 2014

Member

probaby worth returning the original error so that the stack is retained

now.getMilliseconds();

var data = event.data;
if (typeof event.data !== 'string') {

This comment has been minimized.

Copy link
@geek

geek Sep 18, 2014

Member

probably safer to check if its an object and not null

data = JSON.stringify(event.data);
}
catch (e) {
data = 'JSON Error: ' + e.message;

This comment has been minimized.

Copy link
@geek

geek Sep 18, 2014

Member

could fall back to safe stringify


for (var i = 0, il = this._eventQueue.length; i < il; i++) {
var event = this._eventQueue[i];
if (event.event === 'ops') {

This comment has been minimized.

Copy link
@geek

geek Sep 18, 2014

Member

probably easier to maintain if you split each of these conditionals into separate functions: printOps printRequest or formatOps

This comment has been minimized.

Copy link
@arb

arb Sep 18, 2014

Author Contributor

I split the code for reporting a "request" into it's own function. The other three just call the print function and would be one more level of misdirection I don't think we need.


if (event.responsePayload) {
try {
responsePayload = ' response payload: ' + JSON.stringify(event.responsePayload);

This comment has been minimized.

Copy link
@geek

geek Sep 18, 2014

Member

pull the try/catch into a new function for tryStringify that way only that function doesn't have to be optimized and it can also fallback to safe stringify

"async": "0.9.x",
"yargs": "1.3.x",
"json-stringify-safe": "5.0.x"
"yargs": "1.3.x"

This comment has been minimized.

Copy link
@geek

geek Sep 18, 2014

Member

try bossy instead since its stable now

This comment has been minimized.

Copy link
@lloydbenson

lloydbenson Sep 18, 2014

Contributor

is command line even required if we are removing broadcast and replay? If so might be able to just get rid of yargs altogether.

@@ -16,13 +16,14 @@
"node": ">=0.10.x"
},
"dependencies": {
"async": "0.9.x",

This comment has been minimized.

Copy link
@geek

geek Sep 18, 2014

Member

items

@@ -2,6 +2,7 @@

var Lab = require('lab');

This comment has been minimized.

Copy link
@geek

geek Sep 18, 2014

Member

Reverse the order so its in alphabetical order

This comment has been minimized.

Copy link
@arb

arb Sep 18, 2014

Author Contributor

Not sure what you mean. I copied the lab aliases directly from another project...?

var SafeStringify = require('json-stringify-safe');
var Wreck = require('wreck');
var GoodReporter = require('good-reporter');
var Http = require('http');

This comment has been minimized.

Copy link
@geek

geek Sep 18, 2014

Member

Require order should be:

node core
node_modules
local

and try to keep each area in alphabetical order

This comment has been minimized.

Copy link
@arb

arb Sep 18, 2014

Author Contributor

Is that in the style guide somewhere? If it isn't, it should be 😄

@geek geek self-assigned this Sep 25, 2014
geek added a commit that referenced this pull request Sep 25, 2014
Initial draft of version 3
@geek geek merged commit cabc3d8 into hapijs:v3.0.0 Sep 25, 2014
@arb arb deleted the arb:version3 branch Apr 28, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.