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

Invalid condition in WithContext method. #498

Closed
tep opened this issue Oct 20, 2022 · 5 comments · Fixed by #499
Closed

Invalid condition in WithContext method. #498

tep opened this issue Oct 20, 2022 · 5 comments · Fixed by #499

Comments

@tep
Copy link
Contributor

tep commented Oct 20, 2022

Since non-pointer receivers are passed by value, once the receiver for WithContext was reverted to a non-pointer, this condition is guaranteed to always be false.

In other words, the receiver (l) is a copy of the Logger struct from the caller and its address can never be the same as the logger attached to the Context.

Either the receiver should be converted back to a pointer (and different fixes devised for #116 and #400) -- or, this condition should simply be removed and the docs updated to say that the Context will always be replaced no matter what (because that's the actual behavior with the current logic).

[Here's a Go Playground link proving that non-pointer receivers have different addresses from those of their caller]

@rs
Copy link
Owner

rs commented Oct 21, 2022

This is a regression from this commit: 588a61c

@rs
Copy link
Owner

rs commented Oct 21, 2022

Perhaps we should remove the condition altogether.

@tep
Copy link
Contributor Author

tep commented Oct 21, 2022

That's what I'd originally done in PR #499 but then changed it to add back a similar condition using reflect.DeepEqual but, I can always pull it out again.

@rs
Copy link
Owner

rs commented Oct 21, 2022

Ok, I'll prefer removing the condition then.

@tep
Copy link
Contributor Author

tep commented Oct 22, 2022

Done.

#499 should be ready to go.

@rs rs closed this as completed in #499 Oct 22, 2022
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 a pull request may close this issue.

2 participants