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

Jump to bottom

Support configurable delimiter for console encoder #697

Merged
merged 14 commits into from Jul 17, 2020
Merged

Support configurable delimiter for console encoder #697

merged 14 commits into from Jul 17, 2020

Conversation

lixingwang
Copy link
Contributor

@lixingwang lixingwang commented Apr 11, 2019 •
edited by prashantv

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 •
edited

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?

jcorbin
jcorbin suggested changes Apr 16, 2019
View reviewed changes
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
@lixingwang
Merge branch 'master' into console-delimiter
03fd031 
* 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
Optimizing code base on review
a22a38a
@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?

abhinav
abhinav reviewed Jul 3, 2019
View reviewed changes
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
@prashantv prashantv mentioned this pull request Apr 10, 2020
Open
@prashantv prashantv mentioned this pull request Apr 30, 2020
Closed
@CLAassistant
Copy link

CLAassistant commented Jun 13, 2020 •
edited

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.

prashantv
prashantv reviewed Jul 16, 2020
View reviewed changes
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
prashantv
prashantv approved these changes Jul 17, 2020
View reviewed changes
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
@lixingwang @prashantv
Update comment for default seprator
c856182 
Co-authored-by: Prashant Varanasi <github@prashantv.com>
abhinav
abhinav approved these changes Jul 17, 2020
View reviewed changes
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
@lixingwang @abhinav
len would never < 0
75d4af1 
Co-authored-by: Abhinav Gupta <mail@abhinavg.net>
lixingwang and others added 2 commits July 17, 2020 15:46
@lixingwang @abhinav
Change assert nil to NoError for err check
0f67b3b 
Co-authored-by: Abhinav Gupta <mail@abhinavg.net>
@lixingwang @abhinav
remove duplicate msg in test error test case error msg
8460663 
Co-authored-by: Abhinav Gupta <mail@abhinavg.net>
prashantv
prashantv approved these changes Jul 17, 2020
View reviewed changes
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
@lixingwang lixingwang mentioned this pull request Sep 25, 2020
Closed
@dependabot dependabot bot mentioned this pull request Mar 15, 2021
Closed
cgxxv pushed a commit to cgxxv/zap that referenced this pull request Mar 25, 2022
@lixingwang
Support configurable delimiter for console encoder (uber-go#697)
a920f0b 
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.
@github-actions github-actions bot mentioned this pull request Apr 10, 2022
Closed
@github-actions github-actions bot mentioned this pull request Apr 17, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jcorbin jcorbin jcorbin requested changes

@abhinav abhinav abhinav approved these changes

@prashantv prashantv prashantv approved these changes

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@lixingwang @jcorbin @CLAassistant @vosst @abhinav @prashantv