-
Notifications
You must be signed in to change notification settings - Fork 63
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
Changes to datetime formatting #60
Conversation
Formatting a datetime should only pertain to itself and valid datetimes do not contain a space. Should there be a desire to show show a space between the datetime and the process pid in the formatted log, this formatting logic should take place there. Furthermore, the default datetime format is moved to a class variable to allowing this variable to be overwritten by subclasses.
I wrote a formatter for my ruby logger, inheriting from Logger::Formatter, where its responsibility was to format logs to JSON. class MyFormatter < Logger::Formatter
def call(severity, datetime, progname, message)
{ severity: severity, datetime: format_datetime(datetime), progname: progname, message: message }.to_json
end
end
MyFormatter.new.call('INFO', Time.now, nil, 'Test logger')
#=> {"severity":"INFO","datetime":"2021-07-29T09:15:48.031718 ","progname": null,"message":"Test logger"} Notice the trailing space for the value of |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍
@bazay Can you add new test-case about this change? |
@hsbt Added test cases |
Bump @hsbt @luisdavim 🙃 |
Bump @hsbt @luisdavim :) |
Formatting a datetime should only pertain to itself and valid datetimes do not contain a space. Should there be a desire to show show a space between the datetime and the process pid in the formatted log, this formatting logic should take place there.
Furthermore, the default datetime format is moved to a class variable to allowing this variable to be overwritten by subclasses.