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

Avoid processing logs if the level is lower than log.level #39

Closed
wants to merge 1 commit into from

Conversation

DivineGod
Copy link

@DivineGod DivineGod commented Jun 7, 2016

As reported in #36 the log.log function will do processing of log messages even if the log.level is higher than the message level.

This change will break systems expecting the event emitter to emit log messages for every call to log.log as well as having log.record only contain a list of messages that have the appropriate level

As reported in npm#36 the `log.log`
function will do processing of log messages even if the `log.level`
is higher than the message level.

This change will break systems expecting the event emitter to emit
log messages for every call to `log.log` as well as having `log.record`
only contain a list of messages that have the appropriate level
@@ -2,7 +2,7 @@
"author": "Isaac Z. Schlueter <i@izs.me> (http://blog.izs.me/)",
"name": "npmlog",
"description": "logger for npm",
"version": "3.0.0",
"version": "4.0.0",
Copy link
Contributor

Choose a reason for hiding this comment

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

We bump versions as a part of the release process. Please don't bump them in your PR.

@iarna
Copy link
Contributor

iarna commented Jun 8, 2016

I left some inline source comments, but let me back track…

I don't think we can accept this, as npm (the primary consumer of this module) needs log.record to have ALL messages regardless of log level. That's how it produces the npm-debug.log on crashes– that file needs to have all levels of logging regardless of what was printed to the console.

@iarna
Copy link
Contributor

iarna commented Jun 8, 2016

Long term, I suspect we may separate those concerns, at which point we might be able to do something like what you suggested.

@coveralls
Copy link

Coverage Status

Coverage decreased (-17.1%) to 67.285% when pulling 48b8c2a on DivineGod:log-level into 87f6986 on npm:master.

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.

None yet

3 participants