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

ruff server: Improve error message when a command is run on an unavailable document #11823

Merged
merged 2 commits into from
Jun 11, 2024

Conversation

snowsignal
Copy link
Contributor

@snowsignal snowsignal commented Jun 10, 2024

Summary

Fixes #11744.

We now show a distinct popup message when we fail to get a document snapshot during command execution. This message more clearly communicates the issue to the user, instead of a generic "ruff encountered an error" message.

Test Plan

Try running Fix all auto-fixable problems on an incompatible file (for example: settings.json). You should see the following popup message:
Screenshot 2024-06-11 at 11 47 16 AM

@snowsignal snowsignal added the server Related to the LSP server label Jun 10, 2024
Copy link

codspeed-hq bot commented Jun 10, 2024

CodSpeed Performance Report

Merging #11823 will not alter performance

Comparing jane/server/exe-command-warn (94efad9) with main (4e9d771)

Summary

✅ 30 untouched benchmarks

Copy link
Contributor

github-actions bot commented Jun 10, 2024

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

@MichaReiser
Copy link
Member

Have you looked into how other LSPs handle this? I'm kind of surprised that VS Code sends us this request in the first place, considering that we only subscribed for Python files.

@snowsignal
Copy link
Contributor Author

@MichaReiser VS Code makes a command available for every file, unless you add a filter (which isn't possible for us).

Here's how other langauge servers handle this:

Screenshot 2024-06-10 at 9 29 41 AM
  • insta shows a visible error message:
Screenshot 2024-06-10 at 9 33 06 AM

Maybe we should just show a more descriptive error message, then? I'm not too opinionated either way.

@MichaReiser
Copy link
Member

Hmm, interesting. My main concern is that not finding the document is, in most cases, a valid error. For example, it was the root cause for untitled files, and having the server fail instead of silently ignoring the error was a good case. It makes me worried that it will be harder to debug such kind of errors in the future. So yes, maybe showing a more user visible error would help us to still catch the error that something went wrong.

Disclaimer: It's not entirely clear when and how often this kind of requests are sent by VS code. Is it only when a user manually requests e.g. import sorting or does it also happen when they have organize imports enabled on save? Is there a way for us to narrow the requests for which we should apply the more lenient handling?

@snowsignal
Copy link
Contributor Author

Disclaimer: It's not entirely clear when and how often this kind of requests are sent by VS code. Is it only when a user manually requests e.g. import sorting or does it also happen when they have organize imports enabled on save?

It's whenever a command gets run, which usually has to be done through the command palette (so, manual execution). I don't think there's a way to run a command on save built-in to VS Code - usually, those are done with source actions, and are only enabled for a specific language.

Is there a way for us to narrow the requests for which we should apply the more lenient handling?

Not really. I think it would make more sense to just show a descriptive error message that allows the user to narrow down the problem themselves.

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.

It's whenever a command gets run, which usually has to be done through the command palette (so, manual execution).

I would then prefer if we would also show a user facing error in addition to logging a warning to insta similar to insta. It would ensure that we still surface real errors and I don't think it would be annoying, considering that triggering requires a manual user interaction.

@snowsignal snowsignal force-pushed the jane/server/exe-command-warn branch from dae6768 to 94efad9 Compare June 11, 2024 18:46
@snowsignal snowsignal changed the title ruff server: Silently handle unavailable file ruff server: Improve error message when a command is run on an unavailable document Jun 11, 2024
@snowsignal snowsignal enabled auto-merge (squash) June 11, 2024 18:49
@snowsignal snowsignal merged commit 7d5cf18 into main Jun 11, 2024
18 checks passed
@snowsignal snowsignal deleted the jane/server/exe-command-warn branch June 11, 2024 18:50
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.

Ruff: Fix all auto-fixable problems erroring out in VSCode
2 participants