Add debarkify to wrap bark.Logger in zap.Logger #85
Conversation
|
i'll edit the commits/descriptions if this is ok. open questions around unit tests and behavior of a few of the receivers. |
billf
force-pushed
the
debarkify
branch
2 times, most recently
from
1de5331
to
aac1295
Compare
|
updated to use KeyValueMap from #89 |
|
|
||
| func TestDebark_CastNoop(t *testing.T) { | ||
| orig := zap.NewJSON(zap.DebugLevel) | ||
| assert.Equal(t, orig, Debarkify(Barkify(orig))) |
There was a problem hiding this comment.
This will do a deep equals, I think you want to do a pointer check here, assert.True(t, orig == Debarkify(...), ...)
| // descendants. This makes it easy to change the log level at runtime | ||
| // without restarting your application. | ||
| func (z *zapper) SetLevel(l zap.Level) { | ||
| z.lvl = l |
There was a problem hiding this comment.
panic if unknown level?
There was a problem hiding this comment.
I'm fine with not checking for now, we don't check in the underlying jsonLogger implementation either
|
The code looks good, just a few comments on the tests. Let me know if you'd like help with the tests, I can help make some test changes too. Can we also improve the title + description for this change? Thanks @billf |
|
recent update: port #126 changes here as well |
| assert.Equal(t, 0, buf.Len(), "buffer not zero") | ||
| logger.Log(l, "ohai") | ||
| assert.NotEqual(t, 0, buf.Len(), "%q did not log", l) | ||
| buf.Reset() |
There was a problem hiding this comment.
do you need both the buf.Reset() calls? I think the one at the beginning can be removed?
|
lgtm, just a couple of nits, and it's ready to merge. Thanks for being so patient @billf |
| logrusLogger.Out = os.Stdout | ||
| logrusLogger.Formatter = new(logrus.JSONFormatter) | ||
| logrusLogger.Formatter.(*logrus.JSONFormatter).TimestampFormat = "lies" | ||
| barkLogger := bark.NewLoggerFromLogrus(logrusLogger).WithField("errors", 0) |
There was a problem hiding this comment.
errors comes from here and i was just matching the field use above in ExampleBarkify()
There was a problem hiding this comment.
Ah I see, OK makes sense.
|
vet failure seems spurious: |
|
vet can give flaky errors if the package is not installed. We might need to do a |
|
vet issue is addressed in #129 |
|
lgtm, thanks @billf |
No description provided.