Remove field slice in nulllogger to make it comparable #108
Conversation
log/logger.go
Outdated
| @@ -155,7 +155,7 @@ func (l nullLogger) WithFields(newFields ...Field) Logger { | |||
| fields = append(fields, existingFields.ValueAt(i)) | |||
| } | |||
| fields = append(fields, newFields...) | |||
| return nullLogger{Fields(fields)} | |||
| return &nullLogger{Fields(fields)} | |||
There was a problem hiding this comment.
If we return a new logger here, don't we lose the ability to compare NullLoggers?
There was a problem hiding this comment.
Yeah good point, in that case I'll remove the fields from the NullLogger struct since it's not logging anything regardless.
log/logger.go
Outdated
|
|
||
| func (l nullLogger) WithFields(newFields ...Field) Logger { | ||
| func (*nullLogger) Enabled(_ Level) bool { return false } | ||
| func (*nullLogger) Fatalf(msg string, args ...interface{}) {} |
There was a problem hiding this comment.
why not os.Exit(1) here too?
There was a problem hiding this comment.
Not sure, that wasn't my change, I can add it however.
log/logger.go
Outdated
| func (l nullLogger) WithFields(newFields ...Field) Logger { | ||
| func (*nullLogger) Enabled(_ Level) bool { return false } | ||
| func (*nullLogger) Fatalf(msg string, args ...interface{}) {} | ||
| func (*nullLogger) Fatal(msg string) { os.Exit(1) } |
There was a problem hiding this comment.
maybe log the message before exiting
There was a problem hiding this comment.
Not sure if that's consistent with what the null logger does, IMO null logger shouldn't log anything.
There was a problem hiding this comment.
Just feels weird to be crashing the process without any indication why.
There was a problem hiding this comment.
I'd think in that case the user should not be using the null logger and instead use a normal logger like SimpleLogger.
cw9
changed the title
cc @cw9 @robskillington @prateek @jeromefroe
This PR makes null logger a pointer type.