-
Notifications
You must be signed in to change notification settings - Fork 27
Use VS Code's LogOutputChannel for logging #553
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
base: main
Are you sure you want to change the base?
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.
I had no idea this existed! Awesome.
Ah and I mentioned
I think you are right, looking at the |
src/storage.ts
Outdated
|
||
switch (resp.status) { | ||
case 200: { | ||
const rawContentLength = resp.headers["content-length"]; | ||
const contentLength = Number.parseInt(rawContentLength); | ||
if (Number.isNaN(contentLength)) { | ||
this.output.appendLine( | ||
this.output.error( | ||
`Got invalid or missing content length: ${rawContentLength}`, |
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.
`Got invalid or missing content length: ${rawContentLength}`, | |
"Got invalid or missing content length", | |
rawContentLength, |
The methods on LogOutputChannel
all take ...args: any[]
, so we can take advantage of that instead of continuing to use string interpolation
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.
Should I replace all messages that have a dynamic value at the end and remove the trailing :
from the message?
The only issue here is that we cannot guarantee that VS Code is going to log those in the same way in future versions. Is it simply a message + args.join(" ")
?
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 seems this is how VS Code formats the args, primitives are kept as is, objects are stringified and errors made into human readable errors:
function format(args: any, verbose: boolean = false): string {
let result = '';
for (let i = 0; i < args.length; i++) {
let a = args[i];
if (a instanceof Error) {
a = toErrorMessage(a, verbose);
}
if (typeof a === 'object') {
try {
a = JSON.stringify(a);
} catch (e) { }
}
result += (i > 0 ? ' ' : '') + a;
}
return result;
}
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.
that's really seems like the "right" way to do it, I'm pretty convinced they won't change it majorly
Probably requires another review now that I replaced all the calls of |
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.
🎉
HI @EhabY. Thanks for your contribution. Could you also add the change to https://github.com/coder/vscode-coder/blob/main/CHANGELOG.md |
Added the change to the changlog |
Ah right yeah, adding a line to the changelog in the PR is the best way yeah. |
- Use `LogOutputChannel` for the Coder output channel and ensure messages are logged | ||
with the appropriate log 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 word it to be more user-centric, like how does this affect the user and what can/should they do about it?
Something like this maybe:
Respect log levels. In the Coder output panel, you can now select the log level to filter messages.
closes #257
In this PR I create a
LogOutputChannel
instead of the oldOutputChannel
. By default,appendLine
uses theinfo
logging level so I usedinfo
to log most messages, except errors where theerror
level was used.I think all of the info can be downgraded into
debug
level actually, and the errors could be downgraded intowarn
since they are handled or thrown right away. What do you think?