-
Notifications
You must be signed in to change notification settings - Fork 0
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
Basic implementation #1
Conversation
👍 |
a4ce16b
to
c287bb0
Compare
086a964
to
8b94d45
Compare
test/loggerTest.js
Outdated
@@ -0,0 +1 @@ | |||
// TO be added :) |
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.
😞
}; | ||
|
||
module.exports = logger; | ||
module.exports.default = logger; |
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.
why?
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.
Why not? Maybe tomorrow we'll add more methods to export and is good to remember that you can default to one of them 😁
index.js
Outdated
}; | ||
}); | ||
|
||
logger.setLevel = (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.
I would be great if u can provide an example of usage for setLevel
in the readme.
timestamp(), | ||
format.json(), | ||
), | ||
transports: [new transports.Console({ json: 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.
What do you think if we make transports
configurable and the default option to be transports.Console
?
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 think is a great idea! I was considering to do it in a newer PR as it seems complex, not critical and we need the SRE team feedback ;)
test/loggerTest.js
Outdated
const TEST_STRING = 'hello world!'; | ||
const TEST_OBJ = {h:1}; | ||
|
||
describe('DeliveryPromise', () => { |
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.
:D
|
||
// define Logger | ||
const logger = {}; | ||
Object.keys(log.levels).forEach((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.
I think we should find a way to simplify this part, but for now is more than enough.
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.
@rubanour for me is approved 😛
* First commit * Added basic library * Support multi params logging * combine format functions * flat export log level function * Added tests * change test approach a little
No description provided.