Skip to content

{From,With}Context for storing a logger in a context.Context #42

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

Merged
merged 2 commits into from
May 10, 2019

Conversation

mitchellh
Copy link
Contributor

WithContext also supports additional arguments to set fields since this
is a very common operation.

One question @evanphx: should FromContext return L() if no logger is found? That would avoid a nil-check, but I'm not sure if that's the right behavior. For now I return nil.

WithContext also supports additional arguments to set fields since this
is a very common operation.
@mitchellh mitchellh requested a review from evanphx May 10, 2019 03:59
@hashicorp-cla
Copy link

hashicorp-cla commented May 10, 2019

CLA assistant check
All committers have signed the CLA.

@evanphx
Copy link
Contributor

evanphx commented May 10, 2019

A good call on returning L() in the case there is no logger in the context. I think so, because the code is going to want a logger and returning the default one as a fallback seems like the right approach.

@evanphx
Copy link
Contributor

evanphx commented May 10, 2019

Yeah, I think returning L() is the right approach because anyone using FromContext is likely going to be using WithContext and so returning L() will prevent a class of bugs where the context gets rebuilt between the With and From calls.

@mitchellh
Copy link
Contributor Author

Awesome, I agree. Done. Requesting review. 😄

@mitchellh mitchellh merged commit 2f1b231 into master May 10, 2019
@mitchellh mitchellh deleted the f-context branch May 10, 2019 17:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants