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.

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

Update server to return the debug info as string #16214

Merged
merged 1 commit into from
Feb 18, 2025

Conversation

dhruvmanila
Copy link
Member

@dhruvmanila dhruvmanila commented Feb 17, 2025

Summary

This PR updates the ruff.printDebugInformation command to return the info as string in the response. Currently, we send a window/logMessage request with the info but that has the disadvantage that it's not visible to the user directly.

What rust-analyzer does with it's rust-analyzer/status request which returns it as a string which then the client can just display it in a separate window. This is what I'm thinking of doing as well.

Other editors can also benefit from it by directly opening a temporary file with this information that the user can see directly.

There are couple of options here:

  1. Keep using the command, keep the log request and return the string
  2. Keep using the command, remove the log request and return the string
  3. Create a new request similar to rust-analyzer/status which returns a string

This PR implements (1) but I'd want to move towards (2) and remove the log request completely. We haven't advertised it as such so this would only require updating the VS Code extension to handle it by opening a new document with the debug content.

Test plan

For VS Code, refer to astral-sh/ruff-vscode#694.

For Neovim, one could do:

local function execute_ruff_command(command)
  local client = vim.lsp.get_clients({ 
    bufnr = vim.api.nvim_get_current_buf(), 
    name = name,
    method = 'workspace/executeCommand',
  })[1]
  if not client then
    return
  end
  client.request('workspace/executeCommand', {
    command = command,
    arguments = {
      { uri = vim.uri_from_bufnr(0) }
    },
    function(err, result)
      if err then
        -- log error
        return
      end
      vim.print(result)
      -- Or, open a new window with the `result` content
    end
  }

@dhruvmanila dhruvmanila added the server Related to the LSP server label Feb 17, 2025
Copy link
Contributor

github-actions bot commented Feb 17, 2025

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

@dhruvmanila dhruvmanila force-pushed the dhruv/print-debug-info-return-string branch from ab63d88 to c700a50 Compare February 17, 2025 17:09
@dhruvmanila dhruvmanila force-pushed the dhruv/print-debug-info-return-string branch from c700a50 to 8abfba3 Compare February 17, 2025 17:18
Comment on lines 38 to 43
notifier
.notify::<types::notification::LogMessage>(types::LogMessageParams {
message: output,
message: output.clone(),
typ: types::MessageType::INFO,
})
.with_failure_code(ErrorCode::InternalError)?;
Copy link
Member Author

Choose a reason for hiding this comment

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

I prefer to remove the logging part and I don't think it should break anything.

Copy link
Member

Choose a reason for hiding this comment

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

I'm in favor of removing it, but wouldn't it break old VS code clients that don't have the required code to open the document on the client side?

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm yeah. So, that would happen if the user is using a non-latest extension version with the latest ruff in which case I think we should recommend them to upgrade the extension version as it always include improvements and bug fixes. I'm fine keeping this here as well for the time being and we can remove it with 0.10.

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 include a TODO or similar that it should be removed? Or do we want to keep it, considering that other clients can't easily call commands?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think we should remove it in a later version, I'll create an issue for 0.10.

@dhruvmanila dhruvmanila marked this pull request as ready for review February 18, 2025 04:34
Copy link
Member

@MichaReiser MichaReiser left a comment

Choose a reason for hiding this comment

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

Could you add a test plan showing how this works for clients without an editor extension (e.g. neovim)? How does the client know that it has to open a document? Is this the default behavior of a custom command?

@dhruvmanila
Copy link
Member Author

Could you add a test plan showing how this works for clients without an editor extension (e.g. neovim)? How does the client know that it has to open a document? Is this the default behavior of a custom command?

We haven't documented the commands API anywhere nor is it easy to add the argument schema in the capabilities as the arguments are []LspAny (https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#executeCommandParams).

No, this is not the default behavior for a custom command. The user or the plugin author needs to know the argument schema for each command to implement it. As to how to pass in the document URI, that will be dependent on each editor. For example, in VS Code, it's just a vscode.window.activeTextEditor while in Neovim it would be vim.lsp.util.make_text_document_params.

Hmm, that makes me wonder why weren't we using the existing types from lsp_types like VersionedTextDocumentIdentifier instead of Argument. I'll make this as a follow-up change.

Comment on lines 38 to 43
notifier
.notify::<types::notification::LogMessage>(types::LogMessageParams {
message: output,
message: output.clone(),
typ: types::MessageType::INFO,
})
.with_failure_code(ErrorCode::InternalError)?;
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 include a TODO or similar that it should be removed? Or do we want to keep it, considering that other clients can't easily call commands?

@dhruvmanila dhruvmanila merged commit bb2a712 into main Feb 18, 2025
21 checks passed
@dhruvmanila dhruvmanila deleted the dhruv/print-debug-info-return-string branch February 18, 2025 09:46
dcreager added a commit that referenced this pull request Feb 18, 2025
* main: (60 commits)
  [`refurb`] Manual timezone monkeypatching (`FURB162`) (#16113)
  [`pyupgrade`] Do not upgrade functional TypedDicts with private field names to the class-based syntax (`UP013`) (#16219)
  Improve docs for PYI019 (#16229)
  Refactor `CallOutcome` to `Result` (#16161)
  Fix minor punctuation errors (#16228)
  Include document specific debug info (#16215)
  Update server to return the debug info as string (#16214)
  [`airflow`] Group `ImportPathMoved` and `ProviderName` to avoid misusing (`AIR303`) (#16157)
  Fix unstable formatting of trailing end-of-line comments of parenthesized attribute values (#16187)
  Ignore source code actions for a notebook cell (#16154)
  Add FAQ entry for `source.*` code actions in Notebook (#16212)
  red-knot: move symbol lookups in `symbol.rs` (#16152)
  better error messages while loading configuration `extend`s (#15658)
  Format `index.css` (#16207)
  Improve API exposed on `ExprStringLiteral` nodes (#16192)
  Update Rust crate tempfile to v3.17.0 (#16202)
  Update cloudflare/wrangler-action action to v3.14.0 (#16203)
  Update NPM Development dependencies (#16199)
  Update Rust crate smallvec to v1.14.0 (#16201)
  Update Rust crate codspeed-criterion-compat to v2.8.0 (#16200)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
server Related to the LSP server
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants