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

TextFormatter behaviour aligned with stdlib log (fixes #167) #685

Merged
merged 3 commits into from
Aug 26, 2018

Conversation

HectorMalot
Copy link
Contributor

Hoping to help logrus with a small fix for a long-lived issues (#167). I've made the newline behavior consistent with the standard library log package. The fix to TextFormatter removes a single new-line (if present) at the end of the provided Entry.Message. e.g. "testmessage\n\n" will become "testmessage\n". This is the same behavior as log.Printf

Added a test and confirmed existing tests were passing. The benchmark test shows minor impact (up to 1% increase in ns/op), but happy to discuss more efficient options

stdlib `log` adds a newline at the end of a message if none is present,
otherwise does not. Before this change logrus would always add
a newline, resulting in inconsistent behaviour if stdlib log was
replaced with logrus, and a user would e.g. use 'log.printf("test\n")'
@HectorMalot
Copy link
Contributor Author

@dgsb , I've updated the pull request to work with the latest master. Implicit newlines should now work the same as the stdlib. e.g. this prevents the negroni logger middleware from spitting out empty lines when passing logrus instead of the default log.

Would you be open to merge this or alternatively have a discussion on how the PR can be improved?

mkenney added a commit to bdlm/log that referenced this pull request Aug 12, 2018
* sirupsen/logrus/pull/664
* sirupsen/logrus/pull/647
* sirupsen/logrus/pull/687
* sirupsen/logrus/pull/685
* sirupsen/logrus/pull/788 (existed previously)

This also
* updates the string escape logic, all values are now JSON escaped
* fixes an issue with internal properties being included in JSON format
* adds new fields to unit tests (`data` and `caller`)
* minor cleanup of text templates
@dgsb
Copy link
Collaborator

dgsb commented Aug 26, 2018

@HectorMalot thanks for your contribution

@dgsb dgsb merged commit 0908e58 into sirupsen:master Aug 26, 2018
ivucica pushed a commit to ivucica/bdlm-log that referenced this pull request Sep 28, 2020
* sirupsen/logrus/pull/664
* sirupsen/logrus/pull/647
* sirupsen/logrus/pull/687
* sirupsen/logrus/pull/685
* sirupsen/logrus/pull/788 (existed previously)

This also
* updates the string escape logic, all values are now JSON escaped
* fixes an issue with internal properties being included in JSON format
* adds new fields to unit tests (`data` and `caller`)
* minor cleanup of text templates
cgxxv pushed a commit to cgxxv/logrus that referenced this pull request Mar 25, 2022
TextFormatter behaviour aligned with stdlib log (fixes sirupsen#167)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants