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

Implement hover menu support for ruff-server; Issue #10595 #11096

Merged
merged 10 commits into from
Apr 23, 2024

Conversation

nolanking90
Copy link
Contributor

@nolanking90 nolanking90 commented Apr 23, 2024

Summary

Add support for hover menu to ruff_server, as requested in 10595.
Majority of new code is in hover.rs.
I reused the regex from ruff-lsp's implementation. Also reused the format_rule_text function from ruff/src/commands/rule.rs
Added capability registration in server.rs, and added the handler to api.rs.

Test Plan

Tested in NVIM v0.10.0-dev-2582+g2a8cef6bd, configured with lspconfig using the default options (other than cmd pointing to my test build, with options "server" and "--preview"). OS: Ubuntu 24.04, kernel 6.8.0-22.

Copy link

codspeed-hq bot commented Apr 23, 2024

CodSpeed Performance Report

Merging #11096 will not alter performance

Comparing nolanking90:main (8c3130e) with main (455d22c)

Summary

✅ 30 untouched benchmarks

@snowsignal snowsignal added the server Related to the LSP server label Apr 23, 2024
Copy link
Contributor

github-actions bot commented Apr 23, 2024

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

Formatter (stable)

✅ ecosystem check detected no format changes.

Formatter (preview)

✅ ecosystem check detected no format changes.

Copy link
Contributor

@snowsignal snowsignal left a comment

Choose a reason for hiding this comment

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

Thank you for getting this done! You have the honor of being the first outside contributor for ruff server 😄

There are a few things I'd like to improve before we merge this:

  1. Error resiliency - there's a lot of .unwrap()s in here that could potentially panic in a real-world scenario. When I was testing this PR, I noticed that hovering over an incomplete noqa comment would cause the server to panic (for example: # noqa: RUF). I've made some suggestions that use early returns in the place of potentially panicking functions like .unwrap().
  2. Code complexity - sections of this implementation are actually more complicated than they need to be. I've left suggestions on how we can simplify them.

Once again, I appreciate you taking the time to implement this! This is going to be a nifty feature.

Cargo.lock Outdated Show resolved Hide resolved
crates/ruff_server/src/server/api/requests/hover.rs Outdated Show resolved Hide resolved
crates/ruff_server/src/server/api/requests/hover.rs Outdated Show resolved Hide resolved
crates/ruff_server/src/server/api/requests/hover.rs Outdated Show resolved Hide resolved
crates/ruff_server/src/server/api/requests/hover.rs Outdated Show resolved Hide resolved
crates/ruff_server/src/server/api/requests/hover.rs Outdated Show resolved Hide resolved
crates/ruff_server/src/server/api/requests/hover.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@snowsignal snowsignal left a comment

Choose a reason for hiding this comment

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

Excellent! This seems to be working great on VS Code. Good catch with the comparison operators! I have one small nit, but otherwise you are free to merge 👍

crates/ruff_server/src/server/api/requests/hover.rs Outdated Show resolved Hide resolved
@snowsignal
Copy link
Contributor

My bad, I actually have to run the merge. Thanks for bearing with me 🤦‍♀️

@snowsignal snowsignal enabled auto-merge (squash) April 23, 2024 20:07
@snowsignal snowsignal merged commit 7c8c1c7 into astral-sh:main Apr 23, 2024
18 checks passed
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