-
Notifications
You must be signed in to change notification settings - Fork 854
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
Don't use default levels if useOnlyCustomLevels is specified #522
Don't use default levels if useOnlyCustomLevels is specified #522
Conversation
@@ -65,13 +69,14 @@ function setLevel (level) { | |||
if (values[level] === undefined) throw Error('unknown level ' + level) | |||
const preLevelVal = this[levelValSym] | |||
const levelVal = this[levelValSym] = values[level] | |||
const useOnlyCustomLevelsVal = this[useOnlyCustomLevelsSym] |
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.
have you verifyies this property is defined and of the correct value 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.
I had put a console.log
in the code and for my test its true
and for the other tests where it is actually defined. I'm just confused that the test actually returns null
for priority. That's really what I don't understand.
test/custom-levels.test.js
Outdated
is(logger.hasOwnProperty('info'), true) | ||
|
||
logger.info('test') | ||
const { priority } = await once(stream, 'data') |
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 missing the changeLevelName
option.
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.
Ah… Then I misunderstood the changeLevelName
option. And as I only want to test that my level setting is taken over, I will just use the standard name.
What is still missing here? The property check you mentioned in your first comment? |
Oh, is this ready to go? I thought it was a WIP. |
As I mentioned on #507, I was not sure why my unit test was failing, but as you pointed out, I just used the wrong property to compare with. So much for copy & pasting. So from my point of view, I'd say it does what it should: Allowing to redefine standard levels. |
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
This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
As said in #507, I gave it a try to allow to override the standard levels, but for some reason the priority in the test is
null
.