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.

Sign up for GitHub

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

Jump to bottom

Extend WithLazy method to SugaredLogger #1377

Closed
defval opened this issue Oct 31, 2023 · 3 comments · Fixed by #1378
Closed

Extend WithLazy method to SugaredLogger #1377

defval opened this issue Oct 31, 2023 · 3 comments · Fixed by #1378

Comments

@defval
Copy link
Contributor

defval commented Oct 31, 2023 •
edited

Hi, zap maintainers!

Is your feature request related to a problem? Please describe.

The recently added WithLazy method is amazing (PR: #1319). This capability is not yet available for SugaredLogger

Describe the solution you'd like
The straightforward idea is to expose a similar signature from SugaredLogger using WithLazy method :

func (s *SugaredLogger) WithLazy(args ...interface{}) *SugaredLogger {
	return &SugaredLogger{base: s.base.WithLazy(s.sweetenFields(args)...)}
}

There is still overhead from the sweetenFields call, but it's nonetheless much less than using With().

Is this a breaking change?
Nope

If you approve, I'm ready to work on that because it's somewhat blocking us
@r-hang
Copy link
Contributor

r-hang commented Oct 31, 2023

Hey @defval, this feature makes sense to me. Feel free to send us something for review!

@defval defval mentioned this issue Oct 31, 2023
Merged
@defval
Copy link
Contributor Author

defval commented Nov 2, 2023

@r-hang Hey, mate! The PR is ready. Could you take a look at it soon?

@basert
Copy link

basert commented Nov 15, 2023

@r-hang, sorry for the ping, but can you take a look at the implementation?

r-hang pushed a commit that referenced this issue Nov 25, 2023
@defval
Extend WithLazy() method to SugaredLogger (#1378)
fa1b6ae 
Hey there! Resolves #1377

Notes:
- I reused all tests from the `With()` method to work with both `With()`
and `WithLazy()`
- To highlight the differences between `With()` and `WithLazy()`, I
added a test similar to [this
one](https://github.com/uber-go/zap/pull/1319/files#diff-55a2db70576eb079880167200221ea3787a01fa4bbc958503769eba24084d842R179).
I kept only the test cases that clearly show the distinctions

Looking forward to your thoughts!
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Extend WithLazy() method to SugaredLogger defval/zap
3 participants
@basert @defval @r-hang