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: Important errors are now shown as popups #10951

Merged
merged 5 commits into from
Apr 16, 2024

Conversation

snowsignal
Copy link
Contributor

@snowsignal snowsignal commented Apr 15, 2024

Summary

Fixes #10866.

Introduces the show_err_msg! macro which will send a message to be shown as a popup to the client via the window/showMessage LSP method.

Test Plan

Insert various show_err_msg! calls in common code paths (for example, at the beginning of event_loop) and confirm that these messages appear in your editor.

To test that panicking works correctly, add this to the top of the fn run definition in crates/ruff_server/src/server/api/requests/execute_command.rs:

panic!("This should appear");

Then, try running a command like Ruff: Format document from the command palette (Ctrl/Cmd+Shift+P). You should see the following messages appear:

Screenshot 2024-04-16 at 11 20 57 AM

@snowsignal snowsignal added the server Related to the LSP server label Apr 15, 2024
@snowsignal snowsignal force-pushed the jane/server/show-message branch from fac5576 to 6e0b6ca Compare April 15, 2024 10:11
@snowsignal snowsignal requested a review from MichaReiser April 15, 2024 10:11
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.

Having to call both tracing::error and show_err_msg! is a bit verbose but I could see why it is needed. I think my main challenge as a developer would be is to know where I need to use what (not all tracing sides have a show_err_msg call but all show_err_msg calls have a tracing::error call).

Copy link
Contributor

github-actions bot commented Apr 15, 2024

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

@snowsignal snowsignal force-pushed the jane/server/show-message branch from f0837b3 to 6cec603 Compare April 16, 2024 04:18
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.

The code looks good.

To me it's still unclear when to use show_err_msg and tracing:error and why/when I need to use both

Having to call both tracing::error and show_err_msg! is a bit verbose but I could see why it is needed. I think my main challenge as a developer would be is to know where I need to use what (not all tracing sides have a show_err_msg call but all show_err_msg calls have a tracing::error call).

crates/ruff_server/src/message.rs Outdated Show resolved Hide resolved
crates/ruff_server/src/message.rs Outdated Show resolved Hide resolved
@snowsignal
Copy link
Contributor Author

To me it's still unclear when to use show_err_msg and tracing:error and why/when I need to use both

show_err_msg shows a visible popup in the editor, with a user-friendly message. tracing::error is for logging errors in the Output window, usually in a more verbose and technical message.

Not all errors need to be surfaced to the user as visibly as a popup, which is why only some of the error logs also show a message to the user.

@snowsignal snowsignal enabled auto-merge (squash) April 16, 2024 18:24
@snowsignal snowsignal force-pushed the jane/server/show-message branch from d2b1045 to 11c9136 Compare April 16, 2024 18:25
@snowsignal snowsignal merged commit 2882604 into main Apr 16, 2024
17 checks passed
@snowsignal snowsignal deleted the jane/server/show-message branch April 16, 2024 18:32
@snowsignal snowsignal added this to the Ruff Server: Beta milestone Apr 16, 2024
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 server: Improve visibility of LSP errors
3 participants