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

Added SkipLineEnding to zapcore.EncoderConfig #989

Merged
merged 2 commits into from
Nov 9, 2021

Conversation

lruggieri
Copy link
Contributor

@lruggieri lruggieri commented Aug 12, 2021

Settingconfig.EncoderConfig.SkipLineEnding = true avoids 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.

@CLAassistant
Copy link

CLAassistant commented Aug 12, 2021

CLA assistant check
All committers have signed the CLA.

line.AppendString(c.LineEnding)
} else {
line.AppendString(DefaultLineEnding)
if !c.SkipLineEnding {
Copy link
Contributor

@sywhang sywhang Nov 8, 2021

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@lruggieri lruggieri Nov 9, 2021

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

should be handled within the new commit

Copy link
Contributor

@sywhang sywhang left a comment

Choose a reason for hiding this comment

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

@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.

Copy link
Contributor

@sywhang sywhang left a comment

Choose a reason for hiding this comment

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

This looks good to me! Thanks again for the PR and working with us to improve the existing code as well :)

@codecov
Copy link

codecov bot commented Nov 9, 2021

Codecov Report

Merging #989 (6fa2cff) into master (c8e813e) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           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           
Impacted Files Coverage Δ
zapcore/encoder.go 87.09% <ø> (ø)
zapcore/console_encoder.go 100.00% <100.00%> (ø)
zapcore/json_encoder.go 100.00% <100.00%> (ø)
zapcore/clock.go 100.00% <0.00%> (ø)
internal/ztest/clock.go 100.00% <0.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c8e813e...6fa2cff. Read the comment docs.

@manjari25 manjari25 merged commit 7a9e717 into uber-go:master Nov 9, 2021
manjari25 added a commit that referenced this pull request Nov 9, 2021
manjari25 added a commit that referenced this pull request Nov 9, 2021
* Update changelog with text for PR #989

* Edit changelog entry for PR #1011
@abhinav abhinav mentioned this pull request Jan 4, 2022
abhinav added a commit that referenced this pull request Jan 4, 2022
This release v1.20.0 of Zap with a couple new features for customizing
the JSON encoder. Namely,

- support skipping newlines between log statements (#989)
- support changing the reflection JSON encoder (#1039)

Refs GO-1085
abhinav added a commit that referenced this pull request Jan 4, 2022
This release v1.20.0 of Zap with a couple new features for customizing
the JSON encoder. Namely,

- support skipping newlines between log statements (#989)
- support changing the reflection JSON encoder (#1039)

Refs GO-1085
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants