-
Notifications
You must be signed in to change notification settings - Fork 395
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
Fix #405 by providing a better way to configure logger #406
Conversation
Codecov Report
@@ Coverage Diff @@
## master #406 +/- ##
======================================
Coverage 81.5% 81.5%
======================================
Files 7 7
Lines 530 530
Branches 152 152
======================================
Hits 432 432
Misses 70 70
Partials 28 28 Continue to review full report at Codecov.
|
if (typeof logger === 'undefined') { | ||
// Initialize with the default logger | ||
const consoleLogger = new ConsoleLogger(); | ||
consoleLogger.setName('bolt-app'); |
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.
If someone has suggestions for the default name, leave comments here.
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.
Seems fine to me
if (typeof logger === 'undefined') { | ||
// Initialize with the default logger | ||
const consoleLogger = new ConsoleLogger(); | ||
consoleLogger.setName('bolt-app'); |
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.
Seems fine to me
logger = new ConsoleLogger(), | ||
logLevel = LogLevel.INFO, | ||
logger = undefined, | ||
logLevel = undefined, |
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.
Are we still setting LogLevel.INFO
as the default somewhere? Looks like we lost that logic.
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.
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.
Okay rad!
@stevengill Thanks for your review! |
@stevengill This one seems to already look good to you but just in case can you approve this? (no rush) |
Summary
This pull request fixes #405 by changing the initialization of the
App
instances a bit. The changes included in this PR are:logger
,logLevel
arguments ofApp
constructor to distinguish they're explicitly specified or notlogger
withWebClient
asWebClient
has its own logger initialization logics - in other words, only the logLevel will be shared between Bolt apps andWebClient
The behavior
The log output with this fix will be more straight-forward.
If a developer goes with the default settings (default
ConsoleLogger
andLogLevel.INFO
) gives onlylogLevel
to theApp
constructor as below,The logger shows the following outputs.
If a developer wants to customize the logger name, doing as below works.
The output can be like this. The only last line is different from the default logger.
If a developer gives both
logger
andlogLevel
, Bolt gives priority tologLevel
thanlogger
's log level (the log level inlogger
will be overwritten by thelogLevel
while the initialization). This behavior is compatible with the past versions so that I think we should not change it at least this time.Next Steps
I'm sure this improvement should be applied to both Bolt v1 and v2. Once other maintainers approve this change, I will make another PR that brings the same change to the v2 branch later on.
Requirements (place an
x
in each[ ]
)