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.

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

Replace log.Context with log.With/WithPrefix? #474

Closed
willfaught opened this issue Mar 2, 2017 · 6 comments
Closed

Replace log.Context with log.With/WithPrefix? #474

willfaught opened this issue Mar 2, 2017 · 6 comments
Assignees

Comments

@willfaught
Copy link
Contributor

Given the stdlib discussion, is there any interest in replacing log.Context with log.With/WithPrefix?

@ChrisHines
Copy link
Member

I plan to post a PR to do it very soon. That will let us evaluate the overall impact. It will probably break a lot of code, but the migration is pretty straight forward. Also, if we're going to do it, now is the time, before we publish v1.0.0.

@peterbourgon peterbourgon mentioned this issue Mar 3, 2017
6 tasks
@peterbourgon
Copy link
Member

For the record I think this is a straightforward change and should probably make it into 1.0.0, barring any unforeseen weirdness.

@ChrisHines
Copy link
Member

Agreed. My only concern is that unexporting Context somehow cripples a use case that needed to store a *Context, but I don't think we will find that to be the case. I also think that the benefits of this change will turn out to be rather significant from a simplicity point of view. I'll document the pros and cons in the PR which I hope to have up tonight, or by the end of the weekend at the latest.

@ChrisHines ChrisHines self-assigned this Mar 3, 2017
@willfaught
Copy link
Contributor Author

willfaught commented Mar 3, 2017 via email

@ChrisHines ChrisHines mentioned this issue Mar 5, 2017
3 tasks
@ChrisHines
Copy link
Member

Nope, just forgot to close it. Thanks for the ping.

@ereOn
Copy link

ereOn commented Mar 7, 2017

@ChrisHines I just deleted my comment as I realized a release was planned for tomorrow and thought this would be solved at that moment. You are welcome !

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

No branches or pull requests

4 participants