Added SkipLineEnding to zapcore.EncoderConfig #989
Conversation
|
|
zapcore/console_encoder.go
Outdated
| line.AppendString(c.LineEnding) | ||
| } else { | ||
| line.AppendString(DefaultLineEnding) | ||
| if !c.SkipLineEnding { |
There was a problem hiding this comment.
It'd be preferable to change DefaultLineEnding to an empty string instead of doing this because this is essentially adding an additional boolean check even for configs that didn't specify the SkipLineEnding every time we encode.
There was a problem hiding this comment.
Wouldn't changing the default value mean breaking backward compatibility? I suppose there are people expecting to see their logs going to a new line every time, and pulling a change like that would break that behavior, I think.
There was a problem hiding this comment.
sorry, I should've been more clear w/ my comment - I meant that you'd just need to change DefaultLineEnding to an empty string if SkipLineEnding is set to true.
There was a problem hiding this comment.
I see. So merging with your other comment below if would be changing SkipLineEnding to a var and computing the "real" LineEnding at NewXEncoder time, right?
There was a problem hiding this comment.
SkipLineEnding can stay in EncoderConfig; What I'd imagine would change is somewhere along the lines of:
func newJSONEncoder(cfg EncoderConfig, spaced bool) *jsonEncoder {
+ if c.SkipLineEnding {
+ cfg.LineEnding = ""
+ } else if cfg.LineEnding == "" {
+ cfg.LineEnding = DefaultLineEnding
+ }
And in these lines, we'd simply need to do:
line.AppendString(cfg.LineEnding) instead of doing the if checks over and over.
NewConsoleEncoder uses newJSONEncoder so you wouldn't need to change anything else.
There was a problem hiding this comment.
should be handled within the new commit
There was a problem hiding this comment.
@lruggieri thanks for submitting this PR!
There is actually a room for improvement in the existing code as well - if we compute the "real" line ending in NewJsonEncoder or NewConsoleEncoder by checking this if condition just once, we can simply concatenate the "real" line ending here without having to do any if check for each subsequent encoding attempts.
Could you please modify this PR in such a way? Otherwise I can take it from here and make these changes.
Codecov Report
@@ Coverage Diff @@
## master #989 +/- ##
=======================================
Coverage 98.20% 98.20%
=======================================
Files 46 47 +1
Lines 2056 2067 +11
=======================================
+ Hits 2019 2030 +11
Misses 29 29
Partials 8 8
Continue to review full report at Codecov.
|
Setting
config.EncoderConfig.SkipLineEnding = trueavoids adding any further symbol to the end of the line.My use-case is the one of a CGO library internally using zap and redirecting the logs to a custom C/C++ function. The calling function expects a string as output and does not expect a newline, or any other symbol, added.