-
Notifications
You must be signed in to change notification settings - Fork 630
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
fix(log): ensure consistent behavior with @std/log
#5974
Conversation
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 test is failing because debug()
doesn't actually print to the console for some reason. Need to figure out why... Any ideas, @kt3k?
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 you need to call setup and override the default log level to DEBUG like the below:
import * as log from "@std/log";
log.setup({
handlers: {
default: new log.ConsoleHandler("DEBUG"),
},
loggers: {
default: {
level: "DEBUG",
handlers: ["default"],
}
}
});
log.debug("debug");
The default log level is set to "INFO" here
Line 6 in c6d1ab2
export const DEFAULT_LEVEL = "INFO"; |
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.
Are you suggesting we add that to log/debug.ts
? What is the expected behavior of the following script?
import { debug } from "@std/log/debug";
debug("Hello, world!");
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 the expected behavior is noop because the default log level is not low enough to log DEBUG log.
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 have one more thing to check to see if that's true. But if it is true, we should remove the function.
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.
It's noop by default, however the behavior can be changed if the user want to see the debug logs. I personally don't think the removal is a good idea.
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.
Yeah, actually, I'll just document the behavior then.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #5974 +/- ##
=======================================
Coverage 96.76% 96.76%
=======================================
Files 508 508
Lines 39101 39106 +5
Branches 5786 5786
=======================================
+ Hits 37836 37841 +5
Misses 1225 1225
Partials 40 40 ☔ View full report in Codecov by Sentry. |
@kt3k PTAL |
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
Fixes #5972