Skip to content

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

Open
wants to merge 18 commits into
base: main
Choose a base branch
from

Conversation

lionel-
Copy link
Collaborator

@lionel- lionel- commented Jul 3, 2025

The LSP is currently not emitting any log messages because:

  • The default log level is 'off'
  • We're not watching any log level setting so it can't be changed

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:

  • Move log level setting to quarto.server.logLevel. Since it's now a quarto setting, we get notified when it changes.
  • Handle logLevel change events on the server side so the LSP can immediately react to log level changes.
  • Also send logLevel in initializationOptions 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 like logError(), logWarn(), etc, as well as logNotification() and logRequest() 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, which languageserver 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 support LogChannelOutput on the client side (see Adopt to new vscode log output channel API microsoft/vscode-languageserver-node#1116).

# $ git config blame.ignoreRevsFile .git-blame-ignore-revs

# Format vscode extention and LSP files consistently (#748)
a1513effc913e6e59651256d79295da37134dbbf
Copy link
Collaborator Author

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: '^_' }],
Copy link
Collaborator Author

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 _

@@ -0,0 +1,8 @@
install:
Copy link
Collaborator Author

@lionel- lionel- Jul 3, 2025

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.

@lionel- lionel- force-pushed the bugfix/lsp-source-maps branch from 3624d2f to e07fe28 Compare July 4, 2025 06:45
Base automatically changed from bugfix/lsp-source-maps to main July 4, 2025 06:46
Copy link
Collaborator

@juliasilge juliasilge left a 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:

Screenshot 2025-07-04 at 2 46 29 PM

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.

@lionel- lionel- force-pushed the feature/lsp-logger branch from b6d92aa to f675711 Compare July 7, 2025 09:00
@lionel- lionel- force-pushed the feature/lsp-logger branch from f675711 to 7095310 Compare July 7, 2025 09:01
@lionel-
Copy link
Collaborator Author

lionel- commented Jul 7, 2025

@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).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants