reidmorrison / semantic_logger

Semantic Logger is a feature rich logging framework, and replacement for existing Ruby & Rails loggers.
https://logger.rocketjob.io/
Apache License 2.0
873 stars 124 forks source link

Base formatter time_format docs and behavior conflict, with no way to initialize it to nil in the ctor. #259

Closed keithrbennett closed 1 year ago

keithrbennett commented 1 year ago

Environment

Expected Behavior

Unclear, the docs and behavior conflict. I suspect the author's intent was to make self.class.build_time_format(precision) the default time format.

The documentation and behavior for the Base formatter (lib/semantic_logger/formatters/base.rb) conflict. The documentation for the constructor includes:

#     nil:      Returns Empty string for time ( no time is output ).
#     Default: '%Y-%m-%d %H:%M:%S.%<precision>N'

...but the constructor defaults the time_format parameter to nil:

def initialize(time_format: nil, ...

...so it would appear that the time_format defaults to excluding the time from the formatted log string, since the format_time method returns the empty string if the time_format property is nil.

However, in the constructor, the code guarantees that if nil is passed it will be overridden with a non-nil formatter:

@time_format     = time_format || self.class.build_time_format(precision)

Actual Behavior

If nil is passed to the constructor, or the parameter is omitted, the nil is overridden with a non-empty time format.

Pull Request

I am filing a PR with test code that illustrates the conflict, and shows how one can work around it if one wants to set the time_format to nil. (That is, to call thetime_format= method, passing nil, after the formatter is initialized.)

This PR is not (yet) intended to be merged, so I have marked it WIP and not added any entries to the Changelog.

@reidmorrison Which is the desired behavior? Perhaps we should make self.class.build_time_format(precision) the default value for the time_format parameter so that nil can be specified to override it?