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 NewStdLog. (#439) #487

Merged
merged 2 commits into from
Sep 21, 2017

Conversation

delicb
Copy link
Contributor

@delicb delicb commented Aug 2, 2017

With this change new function is added called NewStdLogAt. It has same
behaviour as NewStdLog, but it allows providing log level to use by
returned *log.Logger.

Couple of things to note:

  • it would be much more straight forward to implement this if there is function *zap.Logger.Log(Level, string, …zapcore.Fields) in addition to Debug, Info, etc. I understand if you do not want to introduce it, but if you are opened to it, I can add it and adapt implementation.
  • loggerWriter is changed to accept function to use, so that decision which function should be used is elsewhere. Existing code is compatible, since default was Info and that is passed. This can be other way around - loggerWriter can have both logger and level and decide which function to call during Write execution, but I do not really like idea of executing switch case every time Write is called.
  • Tests are refactored a bit, so that common parts among new tests and existing one TestNewStdLog can be reused. It might be possible to generalize this to table-driven test, but DPanic, Panic and Fatal levels have special handling, so it might be a bit long. I prefer it the way I wrote it, but I am opened to changing it to table driven test if that is preferred.

@CLAassistant
Copy link

CLAassistant commented Aug 2, 2017

CLA assistant check
All committers have signed the CLA.

@codecov
Copy link

codecov bot commented Aug 2, 2017

Codecov Report

Merging #487 into master will increase coverage by 0.02%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #487      +/-   ##
==========================================
+ Coverage   97.16%   97.19%   +0.02%     
==========================================
  Files          37       37              
  Lines        2221     2243      +22     
==========================================
+ Hits         2158     2180      +22     
  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 3216c4c...4fe6514. Read the comment docs.

@akshayjshah
Copy link
Contributor

@delicb Sorry for the long delay reviewing this! Changes and test coverage look great. I'm going to push a commit with a few small nits (e.g., assert.Nil(t, err) is better as assert.NoError(t, err)) and then merge this.

Thank you for the contribution!

@delicb
Copy link
Contributor Author

delicb commented Aug 17, 2017

@akshayjshah Ah, sure, NoError is better. I actually found out that it exists few days ago (after I created this pull request). I can fix that, if you would like.

Thank you for accepting PR.

@akshayjshah
Copy link
Contributor

🤦‍♂️ I completely forgot about this - I'm so sorry, @delicb! I'll merge this today.

delicb and others added 2 commits September 21, 2017 09:22
With this change new function is added called NewStdLogAt. It has same
behaviour as NewStdLog, but it allows providing log level to use by
returned *log.Logger.
Use the `NoError` testing helpers instead of directly comparing errors
to nil.
@akshayjshah akshayjshah merged commit 19915a2 into uber-go:master Sep 21, 2017
@djui
Copy link
Contributor

djui commented Oct 6, 2017

@akshayjshah Was there a specific reason to not additionally make this change (log level configurable) for redirectStdLog, or just to stay focus on the PRs description? Any plans on adding it?

@akshayjshah
Copy link
Contributor

Nope, you're welcome to open a PR with RedirectStdLogAt. The existing function can't start taking extra parameters without breaking backward compatibiliy.

@djui
Copy link
Contributor

djui commented Oct 6, 2017

@akshayjshah Opened #508.

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.

4 participants