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

Support configurable delimiter for console encoder #697

Merged
merged 14 commits into from
Jul 17, 2020

Conversation

lixingwang
Copy link
Contributor

@lixingwang lixingwang commented Apr 11, 2019

It would be nice to have a configurable delimiter for console encoder, tab by default.

For our cases, we prefer to have space as element delimiter.

A custom console delimiter can be set through the consoleSeparator encoder configuration.

@codecov
Copy link

codecov bot commented Apr 11, 2019

Codecov Report

Merging #697 into master will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #697   +/-   ##
=======================================
  Coverage   98.35%   98.35%           
=======================================
  Files          43       43           
  Lines        2366     2368    +2     
=======================================
+ Hits         2327     2329    +2     
  Misses         32       32           
  Partials        7        7           
Impacted Files Coverage Δ
zapcore/encoder.go 88.23% <ø> (ø)
zapcore/console_encoder.go 100.00% <100.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 275c926...d62d218. Read the comment docs.

@jcorbin
Copy link
Contributor

jcorbin commented Apr 11, 2019

Not getting into a deep/full review at this point, but directionally: we probably don't want to add new ways of configuring zap (e.g. environment variable conventions like this). Instead this should just be directly added to the console encoder config, and then integration with environment can be done by the application.

@lixingwang
Copy link
Contributor Author

@jcorbin Agree! env control should done at end-user application. I have made delimiter in encode config, please have a look. thanks.

@lixingwang
Copy link
Contributor Author

@jcorbin or anyone, could you help to take a look this PR please?

zapcore/console_encoder.go Outdated Show resolved Hide resolved
zapcore/encoder.go Outdated Show resolved Hide resolved
zapcore/console_encoder_test.go Outdated Show resolved Hide resolved
* master:
  README: Switch to travis-ci.com for badge (uber-go#709)
  Fix changelog links for 675
  Prep for 1.10.0 release, update CHANGELOG (uber-go#705)
  Add Go 1.12 for Travis (uber-go#707)
  Fix call depth of standard logger in go1.12 (uber-go#706)
  Fix inconsistency between MapObjectEncoder's AddByteString and AppendByteString (uber-go#657)
  Disable HTMLEscape in reflect JSON encoder (uber-go#704)
@lixingwang
Copy link
Contributor Author

@jcorbin Thanks for your details review comments, I updated code, Please have another look

@lixingwang
Copy link
Contributor Author

@jcorbin Any chance to have another look?

@lixingwang
Copy link
Contributor Author

@abhinav @prashantv @jcorbin Could you please have a look?

Copy link
Collaborator

@abhinav abhinav left a comment

Choose a reason for hiding this comment

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

The implementation largely looks fine. Left comments about the tests and a
little about the documentation. Would you mind updating the PR description to
reflect the new changes, please?

Thanks for the change!

zapcore/console_encoder_test.go Outdated Show resolved Hide resolved
zapcore/console_encoder.go Outdated Show resolved Hide resolved
zapcore/encoder.go Outdated Show resolved Hide resolved
zapcore/console_encoder_test.go Outdated Show resolved Hide resolved
zapcore/console_encoder_test.go Outdated Show resolved Hide resolved
zapcore/console_encoder_test.go Outdated Show resolved Hide resolved
zapcore/console_encoder_test.go Outdated Show resolved Hide resolved
@CLAassistant
Copy link

CLAassistant commented Jun 13, 2020

CLA assistant check
All committers have signed the CLA.

@vosst
Copy link

vosst commented Jul 15, 2020

@lixingwang @abhinav Seems like this PR has stalled. Happy to pick up the change unless either of you has objections/plans to complete it 😄

@lixingwang
Copy link
Contributor Author

@vosst thanks for reminding. I am totally lost this PR. let me update it in the coming days.

@lixingwang
Copy link
Contributor Author

Hi, @abhinav and @vosst please approve.

@vosst
Copy link

vosst commented Jul 16, 2020

Hi, @abhinav and @vosst please approve.

Nice, thank you 😄 @prashantv @jcorbin @abhinav probably worth taking another look.

zapcore/console_encoder.go Outdated Show resolved Hide resolved
zapcore/console_encoder.go Outdated Show resolved Hide resolved
zapcore/encoder.go Outdated Show resolved Hide resolved
zapcore/console_encoder_test.go Outdated Show resolved Hide resolved
Copy link
Collaborator

@prashantv prashantv left a comment

Choose a reason for hiding this comment

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

Couple of minor comments on the test, but otherwise looks good, thanks!

zapcore/console_encoder.go Outdated Show resolved Hide resolved
zapcore/console_encoder_test.go Outdated Show resolved Hide resolved
Co-authored-by: Prashant Varanasi <github@prashantv.com>
Copy link
Collaborator

@abhinav abhinav left a comment

Choose a reason for hiding this comment

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

LGTM besides minor suggestions.

zapcore/console_encoder.go Outdated Show resolved Hide resolved
zapcore/console_encoder_test.go Outdated Show resolved Hide resolved
zapcore/console_encoder_test.go Outdated Show resolved Hide resolved
zapcore/console_encoder_test.go Outdated Show resolved Hide resolved
Co-authored-by: Abhinav Gupta <mail@abhinavg.net>
lixingwang and others added 2 commits July 17, 2020 15:46
Co-authored-by: Abhinav Gupta <mail@abhinavg.net>
Co-authored-by: Abhinav Gupta <mail@abhinavg.net>
zapcore/console_encoder_test.go Outdated Show resolved Hide resolved
@prashantv prashantv changed the title support configurable delimiter for console encoder Support configurable delimiter for console encoder Jul 17, 2020
@prashantv prashantv merged commit 53a3870 into uber-go:master Jul 17, 2020
cgxxv pushed a commit to cgxxv/zap that referenced this pull request Mar 25, 2022
It would be nice to have a configurable delimiter for console encoder, tab by default.

For our cases, we prefer to have space as element delimiter.

A custom console delimiter can be set through the `consoleSeparator` encoder configuration.
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.

6 participants