Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Add zaptest logger option to wrap zap.Option's #610

Merged
merged 2 commits into from
Aug 14, 2018

Conversation

iaroslav-ciupin
Copy link
Contributor

Synopsis

This PR adds feature suggested in #609

@CLAassistant
Copy link

CLAassistant commented Jul 17, 2018

CLA assistant check
All committers have signed the CLA.

@codecov
Copy link

codecov bot commented Jul 17, 2018

Codecov Report

Merging #610 into master will increase coverage by 0.14%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #610      +/-   ##
==========================================
+ Coverage   97.23%   97.38%   +0.14%     
==========================================
  Files          40       40              
  Lines        2063     2102      +39     
==========================================
+ Hits         2006     2047      +41     
+ Misses         49       47       -2     
  Partials        8        8
Impacted Files Coverage Δ
zaptest/logger.go 100% <100%> (ø) ⬆️
writer.go 100% <0%> (ø) ⬆️
sink.go 100% <0%> (+7.4%) ⬆️

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 7e7e266...3e7caed. Read the comment docs.

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.

Thanks for the PR, @iaroslav-ciupin! This feature would be great. However, I'd rather not replicate each of the core zap package's options. Instead, I'd rather have the zaptest package provide a generic adapter for the base options:

  • The loggerOptions struct should have a field of zap.Options.
  • Add a function that converts any number of zap.Options to a single zaptest.Option:
func WrapOptions(opts ...zap.Option) LoggerOption { ...implementation...}
  • In NewLogger, we'd pass all the options on the loggerOptions struct to zap.New.

@iaroslav-ciupin iaroslav-ciupin changed the title Add zaptest logger option to add caller Add zaptest logger option to wrap zap.Option's Aug 8, 2018
@iaroslav-ciupin
Copy link
Contributor Author

iaroslav-ciupin commented Aug 8, 2018

@akshayjshah Thank you for your reply! I've adjusted PR and proposal accordingly. Please have a look ;)

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 great!

@akshayjshah akshayjshah merged commit 67bc79d into uber-go:master Aug 14, 2018
cgxxv pushed a commit to cgxxv/zap that referenced this pull request Mar 25, 2022
Add a wrapper that converts any `zap.Option` into a `zaptest.LoggerOption`.
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants