-
Notifications
You must be signed in to change notification settings - Fork 78
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
Add context support #27
Conversation
logr.go
Outdated
return v | ||
} | ||
|
||
return nullLogger{} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll implement whatever the agreed consensus is on #25
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.