Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Adding service that can direct logging to vscode console #377

Merged
merged 13 commits into from
Nov 27, 2020
Merged

Adding service that can direct logging to vscode console #377

merged 13 commits into from
Nov 27, 2020

Conversation

riccardoferretti
Copy link
Collaborator

@riccardoferretti riccardoferretti commented Nov 25, 2020 •
edited
Loading

This should make debugging reported issues much easier

image

ingalless
ingalless reviewed Nov 26, 2020
View reviewed changes
Copy link
Member

@ingalless ingalless left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just one small suggestion! Great work Riccardo, this will really improve our bug reports

packages/foam-core/src/services/logger.ts Outdated

export const consoleLogger: ILogger = createLogger(
(level, message, ...params) => {
if (level != 'off') {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we do level.toLower() !== 'off' here to clear the warnings?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thx for spotting the !=, I fixed it now!

@riccardoferretti
total change of approach with logging
f39d153 
- using classes instead of functions (feels like it fits better the use case)
- using singleton global to not pass logging service around
@riccardoferretti
log it's a util, not a service
19783b8
@riccardoferretti
fixed passing of params
3526d18
@riccardoferretti
split threshold from level
6d69c0e
@riccardoferretti
Copy link
Collaborator Author

@ingalless @jmg-duarte major changes:

  • I went the OO way to model the Logger, I think it fits better with the use case (the code is in the end more readable)
  • I am now using a global Logger object, to avoid passing that around (consider we might want to monitor low level functions, and passing the logger around like that gets out of control)

@riccardoferretti riccardoferretti added this to the 0.7.1 milestone Nov 26, 2020
@riccardoferretti riccardoferretti merged commit 6152e89 into master Nov 27, 2020
@riccardoferretti riccardoferretti deleted the feature/logging branch November 27, 2020 14:38
@riccardoferretti
Copy link
Collaborator Author

@ingalless @jmg-duarte I have merged this to unblock the release, but feel free to still look at the code and leave comments, they can be addressed in a separate PR

@ingalless ingalless self-requested a review November 27, 2020 15:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jmg-duarte jmg-duarte jmg-duarte left review comments

@ingalless ingalless Awaiting requested review from ingalless

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
0.7.1
Development

Successfully merging this pull request may close these issues.
3 participants
@riccardoferretti @jmg-duarte @ingalless