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

Make level configurable for RedirectStdLog #508

Merged
merged 1 commit into from
Oct 30, 2017

Conversation

djui
Copy link
Contributor

@djui djui commented Oct 6, 2017

This change introduces a new function RedirectStdLogAt. It has the same behaviour as RedirectStdLog but allows providing a log level to be used by used *log.Logger.

This change introduces a new function `RedirectStdLogAt`. It has the same
behaviour as `RedirectStdLog` but allows providing a log level to be used
by used `*log.Logger`.
@CLAassistant
Copy link

CLAassistant commented Oct 6, 2017

CLA assistant check
All committers have signed the CLA.

@codecov
Copy link

codecov bot commented Oct 6, 2017

Codecov Report

Merging #508 into master will increase coverage by 0.03%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #508      +/-   ##
==========================================
+ Coverage   97.19%   97.22%   +0.03%     
==========================================
  Files          37       37              
  Lines        2243     2272      +29     
==========================================
+ Hits         2180     2209      +29     
  Misses         56       56              
  Partials        7        7
Impacted Files Coverage Δ
global.go 100% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 35aad58...14028ac. Read the comment docs.

logger := l.WithOptions(AddCallerSkip(_stdLogDefaultDepth + _loggerWriterDepth))
var logFunc func(string, ...zapcore.Field)
switch level {
case DebugLevel:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can imagine a levelToFunc function here to DRY.

Copy link
Contributor

Choose a reason for hiding this comment

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

Agreed! You've been patient enough, though - I'm happy to merge without that and fix it myself :)

@djui
Copy link
Contributor Author

djui commented Oct 12, 2017

@akshayjshah What do you think?

@djui
Copy link
Contributor Author

djui commented Oct 23, 2017

@akshayjshah @billf bump.

@djui
Copy link
Contributor Author

djui commented Oct 30, 2017

@akshayjshah @billf Is there anything I can help out with to get this PR through quicker? :-)

@akshayjshah
Copy link
Contributor

No - I'm really sorry for the absurd delay. Family, work, etc. - reviewing now.

Copy link
Contributor

@akshayjshah akshayjshah left a comment

Choose a reason for hiding this comment

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

Looks good - I'm going to push a commit to DRY things up a bit, but looks good.

Sorry again for the super-long review delay.

logger := l.WithOptions(AddCallerSkip(_stdLogDefaultDepth + _loggerWriterDepth))
var logFunc func(string, ...zapcore.Field)
switch level {
case DebugLevel:
Copy link
Contributor

Choose a reason for hiding this comment

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

Agreed! You've been patient enough, though - I'm happy to merge without that and fix it myself :)

@akshayjshah akshayjshah merged commit 54bf686 into uber-go:master Oct 30, 2017
@djui
Copy link
Contributor Author

djui commented Oct 30, 2017

@akshayjshah Thanks guys! 🙇 and thanks for the good work on it.

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

Successfully merging this pull request may close these issues.

4 participants