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

Refactor to reuse Log method #1128

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

sywhang
Copy link
Contributor

@sywhang sywhang commented Jul 12, 2022

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.

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
Copy link

codecov bot commented Jul 12, 2022

Codecov Report

Merging #1128 (cbe4c48) into master (78c80fb) will decrease coverage by 0.10%.
Report is 94 commits behind head on master.
The diff coverage is 100.00%.

@@            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     
Files Coverage Δ
logger.go 96.49% <100.00%> (-0.18%) ⬇️
sugar.go 100.00% <100.00%> (ø)

... and 1 file with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Comment on lines +193 to +196
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...)
}
Copy link
Member

Choose a reason for hiding this comment

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

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)
Copy link
Member

Choose a reason for hiding this comment

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

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)
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: https://github.com/uber-go/guide/blob/master/style.md#avoid-naked-parameters

Suggested change
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 {
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: should the log call add +1 to the skip so that callers don't need to specify 2

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

Successfully merging this pull request may close these issues.

3 participants