-
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
refactor: singleton pattern for logging #47
Conversation
export const createKyselyPostgresDb = (config: DatabaseConfig): Kysely<Database> => { | ||
export const createKyselyPostgresDb = ( | ||
config: DatabaseConfig, | ||
logger?: ILogger, |
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.
Optional refactoring suggestion:
When we were working on ebo, Yaco had a nice suggestion to avoid having an optional logger everywhere--you can use the null object design pattern and so if the logger doesn't exist it simply won't log rather than having to handle all the cases it might be null. You can check out the ebo agent repo for the implementation
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.
ooo love this, will implement this
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.
well actually will take it into account when we implement Notifications (as you did in EBO), for the Notification service
🤖 Linear
Closes GIT-204
Description
We re-analyzed the multiple instances of Logger and agreed that is best to have a singleton. So we rollbacked to the original solution of a singleton instance and improved its capabilities with a context object to log more information
Checklist before requesting a review