Refactor to reuse Log method #1128
Conversation
This refactors the implementation of the Log method to reuse it in the other level variants of the method (i.e. `Debug`, `Info`, etc.). To do this, a skip argument was added to `check` to allow its callers to specify how many frames to skip in the log, and a private `log` method was added which calls this `check` method with the appropriate frame skip count.
Codecov Report
@@ Coverage Diff @@
## master #1128 +/- ##
==========================================
- Coverage 98.32% 98.22% -0.10%
==========================================
Files 49 49
Lines 2147 2141 -6
==========================================
- Hits 2111 2103 -8
- Misses 28 29 +1
- Partials 8 9 +1
... and 1 file with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
| func (log *Logger) log(lvl zapcore.Level, msg string, skip int, fields ...Field) { | ||
| if ce := log.check(lvl, msg, skip); ce != nil { | ||
| ce.Write(fields...) | ||
| } |
There was a problem hiding this comment.
nit: Let's move Logger.log below the exported definitions (maybe right above or below Logger.check).
| @@ -187,47 +187,46 @@ func (log *Logger) With(fields ...Field) *Logger { | |||
| // is enabled. It's a completely optional optimization; in high-performance | |||
| // applications, Check can help avoid allocating a slice to hold fields. | |||
| func (log *Logger) Check(lvl zapcore.Level, msg string) *zapcore.CheckedEntry { | |||
| return log.check(lvl, msg) | |||
| return log.check(lvl, msg, 0) | |||
There was a problem hiding this comment.
I might be misreading, but should this be 1 instead? And for log methods below, should those be 3? (There was previously a const offset of 2, but we've since added another intermediate call.)
Can we add a test to validate the skips?
| @@ -187,47 +187,46 @@ func (log *Logger) With(fields ...Field) *Logger { | |||
| // is enabled. It's a completely optional optimization; in high-performance | |||
| // applications, Check can help avoid allocating a slice to hold fields. | |||
| func (log *Logger) Check(lvl zapcore.Level, msg string) *zapcore.CheckedEntry { | |||
| return log.check(lvl, msg) | |||
| return log.check(lvl, msg, 0) | |||
There was a problem hiding this comment.
nit: https://github.com/uber-go/guide/blob/master/style.md#avoid-naked-parameters
| return log.check(lvl, msg, 0) | |
| return log.check(lvl, msg, 0 /* skip */) |
same for all below callsites where we pass a number
| } | ||
|
|
||
| func (log *Logger) log(lvl zapcore.Level, msg string, skip int, fields ...Field) { | ||
| if ce := log.check(lvl, msg, skip); ce != nil { |
There was a problem hiding this comment.
nit: should the log call add +1 to the skip so that callers don't need to specify 2
This refactors the implementation of the Log method to reuse it
in the other level variants of the method (i.e.
Debug,Info, etc.).To do this, a skip argument was added to
checkto allow its callers tospecify how many frames to skip in the log, and a private
logmethodwas added which calls this
checkmethod with the appropriate frameskip count.