-
Notifications
You must be signed in to change notification settings - Fork 49
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
Deprecate EventLogging and Warning* functions #62
Conversation
License: MIT Signed-off-by: Jakub Sztandera <kubuxu@protonmail.ch>
License: MIT Signed-off-by: Jakub Sztandera <kubuxu@protonmail.ch>
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 really wish we could just use one logger everywhere so we could associate events with logs... but oh well).
log.go
Outdated
@@ -41,6 +43,9 @@ type StandardLogger interface { | |||
type EventLogger interface { | |||
StandardLogger | |||
|
|||
Warn(args ...interface{}) |
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.
Why did we move these?
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 can't require them on StandardLogger as why/log
doesn't implement them.
License: MIT Signed-off-by: Jakub Sztandera <kubuxu@protonmail.ch>
I figured out how to make the interface nicer. |
With Zap we can switch Event logs to structured logs. |
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.
Code looks good to me, not sure why CI is failing.
I can't reproduce it. |
This is first set toward migrating this pacakage away from event logging and
toward Zap.
The aim is for this pacakge to be a manager for Zap loggers.