-
Notifications
You must be signed in to change notification settings - Fork 38
Fix logging and configuration watching in LSP #750
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
# $ git config blame.ignoreRevsFile .git-blame-ignore-revs | ||
|
||
# Format vscode extention and LSP files consistently (#748) | ||
a1513effc913e6e59651256d79295da37134dbbf |
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 should skip the format commit in blame, at least on github
@@ -1,4 +1,7 @@ | |||
module.exports = { | |||
root: true, | |||
extends: ["custom-server"], | |||
rules: { | |||
'@typescript-eslint/no-unused-vars': ['error', { argsIgnorePattern: '^_' }], |
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.
Don't lint unused variables prefixed with _
apps/vscode/justfile
Outdated
@@ -0,0 +1,8 @@ | |||
install: |
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.
Taking example on Biome, we've started using the just
command runner in Ark and Air. I've started one here to easily update the dev Quarto extension in Code and Positron.
3624d2f
to
e07fe28
Compare
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 I am seeing when I build from this PR is a bit confusing:

I see Quarto LSP logging coming through 🎉 but the default for this setting seems to be warning while I see even trace messages coming through. What do you think might be happening here?
Also, like I said in #753 I think the just
file should be at the root with the other build and dev scripts.
b6d92aa
to
f675711
Compare
f675711
to
7095310
Compare
7a14238
to
b260250
Compare
@juliasilge Thanks for catching that, this should now work as expected with the user log level taking effect immediately on init. I've removed the justfile (it was predating the contributing PR). |
The LSP is currently not emitting any log messages because:
I've also noticed the following issues:
The log level setting on the LSP side is
markdown.server.log
which is a remnant from copying over the Markdown LSP.The LSP log levels are currently "off", "debug", and "trace". I think we're missing "warn" and "error" for important user messages, and "info" for benign user messages. If we have these, then we should probably remove "off" and have the least verbose level set to "error" (this is what we do in Ark for instance).
Main fixes in this PR:
quarto.server.logLevel
. Since it's now aquarto
setting, we get notified when it changes.logLevel
change events on the server side so the LSP can immediately react to log level changes.logLevel
ininitializationOptions
so the LSP can use the level set by the user as early as possible.Adjacent fixes and cleanups:
Refactor log levels to take error, warn, info, debug, and trace levels.
Set default trace level to
warn
.Add
log()
wrappers likelogError()
,logWarn()
, etc, as well aslogNotification()
andlogRequest()
to log at trace level received LSP messages.Call the latter in message handlers to increase the coverage of LSP events at trace level, which is very valuable for debugging.
Minor: The logger class currently emits messages via
console.log
, whichlanguageserver
redirects to the client when the connection is of type StdIo (see // https://github.com/microsoft/vscode-languageserver-node/blob/df56e720/server/src/node/main.ts#L262-L264). I changed this to explicitly send messages via our connection object instead, which should also work if someone needs to connect to the LSP via TCP. This will also allow us to use more accurate LSP loggers once we switch to languageclient 10 which will supportLogChannelOutput
on the client side (see Adopt to new vscode log output channel API microsoft/vscode-languageserver-node#1116).