-
Notifications
You must be signed in to change notification settings - Fork 775
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
add line number support #879
Conversation
Codecov Report
@@ Coverage Diff @@
## master #879 +/- ##
==========================================
- Coverage 98.03% 97.98% -0.06%
==========================================
Files 25 25
Lines 968 991 +23
==========================================
+ Hits 949 971 +22
- Misses 19 20 +1
Continue to review full report at Codecov.
|
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.
This is great - thanks for working on it. Apart from the spelling mistake in the function name, the only other thing I could think of that might need doing is to add a global config parameter to turn stack traces on or off for all loggers. What do you think?
What I imagine is that a user will want all their logs to have the line numbers, and so would expect to do:
log4js.configure({
appenders: ...,
categories: ...,
enableCallStack: true
});
or maybe if we wanted something a bit more granular:
log4js.configure({
appenders: ...,
categories: {
default: { appenders: [...], level: 'debug', enableCallStack: true }
}
});
What do you think? I'm happy to accept this PR as it is now, it will be useful to a lot of users - but if you've got the time/energy to do a little bit more I think we could make it even more useful.
lib/logger.js
Outdated
@@ -67,6 +91,15 @@ class Logger { | |||
clearContext() { | |||
this.context = {}; | |||
} | |||
|
|||
enabelCallStack(bool = true) { |
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.
Spelling mistake here - should be enableCallStack
I prefer second XD |
@nomiddlename I saw in /lib/log4js:126 |
@nomiddlename
|
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.
Thanks for making the changes. I agree with you about the category level config being better - let's not bother with the global one. I made some comments about making logger.useCallStack
a property following the same pattern as the logger.level
property.
lib/log4js.js
Outdated
@@ -33,6 +33,16 @@ const clustering = require('./clustering'); | |||
const connectLogger = require('./connect-logger'); | |||
|
|||
let enabled = false; | |||
let gCallStackEnabled = false; | |||
const loggers = new Map(); |
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.
I don't think you need to cache loggers. They were designed to be lightweight. It's not a big deal to create lots of them.
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.
fixed
lib/logger.js
Outdated
@@ -67,6 +90,15 @@ class Logger { | |||
clearContext() { | |||
this.context = {}; | |||
} | |||
|
|||
enableCallStack(bool = true) { |
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.
enableCallStack
with an argument seems confusing to me. This might be better as a property of the Logger class, with get
and set
functions which validate the boolean value (similar to how the level
property is defined on Logger). This would also avoid having to cache Loggers by setting the useCallStack
value at the category level (like how the log level is set at the category level).
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.
But, that way will influence after new Logger(), not respect to the configuration
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.
Take a look at how the level
property is defined - it always looks up the level in the category configuration, so it applies to all loggers.
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.
yup, applies to all same category loggers.
lib/log4js.js
Outdated
@@ -140,7 +158,8 @@ const log4js = { | |||
shutdown, | |||
connectLogger, | |||
levels, | |||
addLayout: layouts.addLayout | |||
addLayout: layouts.addLayout, | |||
useCallStack, |
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.
I don't think you need the global useCallStack - keeping it at the category level seems cleaner and simpler.
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.
fixed
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.
This is great. Thanks for making the changes.
Published to NPM in 4.3.0 |
Would it be better to implement it with babel plugin? |
It looks like these fields didn't make it into the type description for |
enableCallStack(boolean)
%f
fileName%l
lineNumber%o
columnNumber%s
call stack