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

export Logger.Sink #66

Merged
merged 1 commit into from
Aug 13, 2021
Merged

export Logger.Sink #66

merged 1 commit into from
Aug 13, 2021

Conversation

pohly
Copy link
Contributor

@pohly pohly commented Aug 11, 2021

This is necessary to support the "breaking glass" use cases. Two
examples for that, access to the underlying implementation and a
custom With* function, get implemented for funcr and tested with
runable examples.

Fixes: #63

Copy link
Contributor

@thockin thockin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks OK overall, small points

funcr/funcr.go Outdated Show resolved Hide resolved
funcr/funcr.go Outdated Show resolved Hide resolved
logr.go Outdated Show resolved Hide resolved
logr.go Outdated Show resolved Hide resolved
@thockin
Copy link
Contributor

thockin commented Aug 12, 2021

This seems good. Before I merge - can we think of any way this could cause problems or compat breaks? I don't think so, but these things are subtle.

@pohly
Copy link
Contributor Author

pohly commented Aug 12, 2021

That's why I filed issue #67 and referenced the apidiff testing from klog 😁 I had a quick look right now and tried to run apidiff manually, but must have done something wrong because it didn't even warn about some intentional API break that I introduced for testing the tool.

FWIW, I also think the change is safe.

@pohly
Copy link
Contributor Author

pohly commented Aug 12, 2021

apidiff doesn't seem to work with Go 1.16. I got output only after building it with Go 1.13.3. This dampens my enthusiasm for the tool a bit - I had expected it to keep up with changes in Go.

But once it runs, it seems to work. It correctly detected the intentional API breaks and for this PR reports:

Compatible changes:
- Logger.Sink: added

@thockin
Copy link
Contributor

thockin commented Aug 12, 2021

hm it seems to work for me. let me see.

@thockin thockin closed this Aug 12, 2021
@thockin thockin reopened this Aug 12, 2021
logr.go Outdated Show resolved Hide resolved
This is necessary to support the "breaking class" use cases. One
example for that, access to the underlying implementation, gets
implemented for funcr and tested with a runable example.
@thockin thockin merged commit 2245c2f into go-logr:master Aug 13, 2021
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.

extending a Logger
2 participants