support hijacking other-than-"std" log.logger #344
Conversation
|
@flisky, thanks for your PR! By analyzing the history of the files in this pull request, we identified @peter-edge, @akshayjshah and @skipor to be potential reviewers. |
|
|
There was a problem hiding this comment.
Thanks for the PR, @flisky - this looks great. I've left a few nits - once they're cleaned up, we can land this.
Note to myself: before landing this, we'll also need to reword the documentation comment for RedirectStdLog to clarify that it only works for the package-global standard logger.
global_test.go
Outdated
| assert.Equal(t, []observer.LoggedEntry{{ | ||
| Entry: zapcore.Entry{Message: "redirected"}, | ||
| Context: []zapcore.Field{}, | ||
| }}, logs.AllUntimed(), "Unexpected global log output.") |
There was a problem hiding this comment.
nit: the output here isn't global. Please adjust the error message to clarify :)
global.go
Outdated
| const ( | ||
| stdLogDefaultDepth = 4 | ||
| loggerWriterDepth = 1 | ||
| ) |
There was a problem hiding this comment.
Now that we're using these constants in two places, let's make them global. Please remember that this project's style underscore-prefixes globals, so this will be
const (
_stdLogDefaultDepth = 4
_loggerWriterDepth = 1
)
|
Thank you, and please take another look. |
|
Hrm, GitHub appears to be interpreting raw git actions oddly. Will re-open in another PR and merge. |
In addition to letting users redirect the output of the standard library's package-global log functions, we should also support wrapping a zap logger in a log.Logger. This is the same PR as #344, which I somehow closed by mistake :/
Sometimes other library requires a log.Logger instance,
but the builtin log doesn't expose the std variable.