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

Closes #357. Reporter name attribute used in errors #380

Merged
merged 4 commits into from Sep 2, 2015

Conversation

@kevinmstephens
Copy link
Contributor

kevinmstephens commented Sep 1, 2015

@arb

As an alternative to #375.

};
var reporterName = Hoek.reach(item, 'reporter.attributes.name', reachOptions);
reporterName = reporterName || Hoek.reach(item, 'reporter.attributes.pkg.name', reachOptions);
if (typeof reporterName === 'string' && reporterName.length > 0) {

This comment has been minimized.

Copy link
@arb

arb Sep 1, 2015

Contributor

I think just a truth check here would be fine. If you're going to be silly and have your reporter name be function () {} then you don't deserve to see a nice name during the error phase.

Items.serial(this.settings.reporters, function (item, next) {

var reporter;

// Use hint of reporter index
var errorHint = self.settings.reporters.length > 1 ? '(' + reportersIndex++ + ')' : '';

This comment has been minimized.

Copy link
@arb

arb Sep 1, 2015

Contributor

Couldn't you just start reportersIndex at 1 instead of having a conditional check in every iteration? Also since these errors are for developers, couldn't we just use 0-based indexing like JS already uses?

This comment has been minimized.

Copy link
@kevinmstephens

kevinmstephens Sep 1, 2015

Author Contributor

It does start at 0, this assert proves as much: kevinmstephens@16036d2#diff-f24923ecc00214a4a695c4c4b28d23cbR341

Would you like to remove the logic that omits the hint index in the case of only one reporter?

Hoek.assert(typeof item.reporter === 'function', 'reporter key must be a constructor function');
Hoek.assert(typeof item.events === 'object', 'reporter must specify events to filter on');
// attempt to upgrade hint to reporter name
var reachOptions = {

This comment has been minimized.

Copy link
@arb

arb Sep 1, 2015

Contributor

Do we really need this?

@arb arb self-assigned this Sep 1, 2015
@arb arb added the feature label Sep 1, 2015
@kevinmstephens

This comment has been minimized.

Copy link
Contributor Author

kevinmstephens commented Sep 1, 2015

@arb Ready for re-review.

Hoek.assert(typeof item.reporter === 'function', 'reporter key must be a constructor function');
Hoek.assert(typeof item.events === 'object', 'reporter must specify events to filter on');
// attempt to upgrade hint to reporter name
var attributes = item.reporter.attributes || {};

This comment has been minimized.

Copy link
@arb

arb Sep 1, 2015

Contributor

Sorry I wasn't clear - when I said "do we need this?" I meant the options into Hoek.reach. Use Hoek.reach instead as it's much easier to read; I just don't think we need any options in there.

This comment has been minimized.

Copy link
@kevinmstephens

kevinmstephens Sep 1, 2015

Author Contributor

Yes we do need this. By default Hoek.reach won't traverse functions, so this options is set to allow traversing across the constructor.

Items.serial(this.settings.reporters, function (item, next) {

var reporter;

// Use hint of reporter index
var errorHint = '(' + reporterIndex++ + ')';

This comment has been minimized.

Copy link
@arb

arb Sep 1, 2015

Contributor

Can you split into two lines? This is hard to read there are 4 plus signs very close to each other.

@kevinmstephens

This comment has been minimized.

Copy link
Contributor Author

kevinmstephens commented Sep 1, 2015

@arb Ready for re-review

@arb arb added this to the 6.4.0 milestone Sep 1, 2015

monitor = new Monitor(new Hapi.Server(), options);
monitor.start(Hoek.ignore);
}).to.throw('reporter must specify events to filter on (test-reporter-two)');

This comment has been minimized.

Copy link
@arb

arb Sep 1, 2015

Contributor

Can we change the message so it reads "test-reporter-two must specify events to filter on" and if there isn't a name, "reporter [1] must specify events to filter on" and have the number be 0-based like the JS array?

@kevinmstephens

This comment has been minimized.

Copy link
Contributor Author

kevinmstephens commented Sep 1, 2015

@arb Changes made to error string format.

@arb arb mentioned this pull request Sep 2, 2015
arb added a commit that referenced this pull request Sep 2, 2015
Closes #357. Reporter name attribute used in errors
@arb arb merged commit 76d7f3b into hapijs:master Sep 2, 2015
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@arb

This comment has been minimized.

Copy link
Contributor

arb commented Sep 2, 2015

Thanks for contributing 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.