Remove use of logging methods in conditional tests #677
Conversation
| if (log.trace) { log.trace('data event: %s', util.inspect(data)) } | ||
| log.trace('data event: %s', util.inspect(data)) |
There was a problem hiding this comment.
Was the if statement supposed to check for the existence of the function? If the user does not provide a logger (or a logger that does not provide a trace method) does this throw an error?
It looks like some of the if statements check the return value and some check the function (like this one did).
There was a problem hiding this comment.
Good points. Integration with logging packages isn't terribly strict, though it is functional. The risk is pushed onto the developer that wants to make use of the built-in calls.
Was the if statement supposed to check for the existence of the function?
If no log is provided, an abstract-logger instance is used to fill out the expected methods. These express nothing and return void, but do prevent errors.
If the user does not provide a logger (or a logger that does not provide a trace method) does this throw an error?
There is no guarantee or test that a provided logging instance will have the expected methods or that its methods will have the expected signatures/behavior. That does put pressure on developers to use compatible loggers, but that will be the case with or without this PR.
It looks like some of the if statements check the return value and some check the function (like this one did).
Note that there's an inconsistency in how calls on the log object are being made. In some cases there is no 'if' check at all.
The single log.trace check (vs. log.trace()) happens in a type that inherits the same log object used elsewhere, so that appears to be an inconsistency as well.
There was a problem hiding this comment.
There is no guarantee or test that a provided logging instance will have the expected methods or that its methods will have the expected signatures/behavior.
On the one hand this could be a problem in theory, on the other hand this hasn't been a problem yet so maybe it isn't in practice.
Note that there's an inconsistency in how calls on the log object are being made. In some cases there is no 'if' check at all.
Yes making it consistent seems like a good reason for this PR.
|
@UziTech when you get a chance, test out your new publishing privileges with a new patch release (2.2.1). |
|
Please include a minimal reproducible example |
To address issue #676 .
Use of log methods (e.g.
log.trace()) in conditional tests prior to actually logging messages yields undesirable behavior when using Pino as an alternative to Bunyan (and potentially other Bunyan-like packages).Additionally, the default behavior for Bunyan and similar loggers is to filter events based on instance config options. The conditionals are not necessary.