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

FEAT: Support ignorePaths option to exclude certain paths from logging. #30

Merged
merged 4 commits into from
Nov 15, 2017

Conversation

deedubs
Copy link
Contributor

@deedubs deedubs commented Nov 13, 2017

This is my first pass at this.

My goals were:

  • Little to no impact if not used
  • Preprocess as much as possible

closes #26

@coveralls
Copy link

coveralls commented Nov 13, 2017

Coverage Status

Coverage decreased (-1.2%) to 97.035% when pulling 5005ea4 on deedubs:feature/26-Add_ignorePaths_option into 91b8e03 on pinojs:master.

Copy link
Collaborator

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add a unit test, and use https://www.npmjs.com/package/abstract-logging

@deedubs
Copy link
Contributor Author

deedubs commented Nov 13, 2017

@mcollina Definitely plan to add coverage, just wanted to give you early look at the implementation.

@coveralls
Copy link

coveralls commented Nov 13, 2017

Coverage Status

Coverage increased (+0.1%) to 98.323% when pulling 3288083 on deedubs:feature/26-Add_ignorePaths_option into 91b8e03 on pinojs:master.

@deedubs
Copy link
Contributor Author

deedubs commented Nov 14, 2017

@mcollina I'm not sure what you're referring to with the request for abstract-logging do you want to add logging somewhere?

I've got the text coverage added.

index.js Outdated

for (let i = 0; i < levels.length; i++) {
logger[levels[i]] = noop
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

instead of building a null logger every time, you can use https://www.npmjs.com/package/abstract-logging.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I see now. I was trying to keep hapi-pino in that golden single external dep happy place a while longer. I can pull that in if you'd prefer.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add the dependency ;)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@mcollina mcollina merged commit 47b2118 into hapijs:master Nov 15, 2017
@deedubs deedubs deleted the feature/26-Add_ignorePaths_option branch November 15, 2017 19:32
@felixheck
Copy link
Collaborator

@deedubs do you wanna add the option to the README.md including a short description and fire another PR? :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support option for ignoring paths
4 participants