Add context support #27
Conversation
logr.go
Outdated
| return v | ||
| } | ||
|
|
||
| return nullLogger{} |
There was a problem hiding this comment.
From #25:
I think the contract here should be "returns the context's Logger or nil if no Logger is in the context". Libs which expect a Logger should say so and make callers provide one. I know DI is not really Go-ish, but it has saved my butt many times.
Offering a NullLogger (#26) and automatically using it are 2 very different things.
Just return nil here and make the comment clear.
There was a problem hiding this comment.
I think a disabled logger is a better experience. Then there is no difference for library code between getting a logger that has been disabled manually due to V or not having a logger in the context. Otherwise we force everyone to perform a nil check every time they obtain a logger from a context.
There was a problem hiding this comment.
Github email put this on the conversation rather than here.
I understand that you think that. We disagree on this point and that is
OK. I'm a fan of low-magic and obvious errors.
If it turns out I am wrong (it happens at least daily, ask my kids!) this
can probably be changed with relative ease.
There was a problem hiding this comment.
As mentioned in #25: we could add simple FromContext() and FromContextOrNull() which does what you describe, I guess.
There was a problem hiding this comment.
I'll implement whatever the agreed consensus is on #25
|
Updated to the consensus in #25, PTAL |
|
Just one super tiny comment change and I am happy |
|
Thanks! |
|
Hi, when will this be released? My project depends on this feature, happy to know the release date. |
|
… On Sun, Nov 1, 2020 at 7:42 PM bbiao ***@***.***> wrote:
Hi, when will this be released? My project depends on this feature, happy
to know the release date.
—
You are receiving this because you modified the open/close state.
Reply to this email directly, view it on GitHub
<#27 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABKWAVH52E2TCTDQUEYIU3TSNYTA5ANCNFSM4SYU7ADA>
.
|
Fixes #25
Cannot be merged until there is a null logger implementation, see #26
I can add as part of this PR if that helps.