-
Notifications
You must be signed in to change notification settings - Fork 1.6k
[ty] Add python.ty.disableLanguageServices
config
#18230
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
Conversation
|
c0db4f2
to
f335a4d
Compare
#[derive(Debug, Deserialize, Default)] | ||
#[cfg_attr(test, derive(PartialEq, Eq))] | ||
#[serde(rename_all = "camelCase")] | ||
struct Python { | ||
ty: Option<Ty>, | ||
} | ||
|
||
#[derive(Debug, Deserialize, Default)] | ||
#[cfg_attr(test, derive(PartialEq, Eq))] | ||
#[serde(rename_all = "camelCase")] | ||
struct Ty { | ||
disable_language_services: Option<bool>, | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
An alternative here is to avoid introducing "Python" struct and directly use disableLanguageServices
in ClientSettings
like:
struct ClientSettings {
disable_language_services: Option<bool>
}
The disadvantage for this is that in VS Code the setting would be python.ty.disableLanguageServices
while in other editors it would just be disableLanguageServices
.
This PR keeps it consistent across editors and this is what we want as well because once workspace/didChangeConfiguration
notification support is added, we'd directly query the client (editor) to get this setting value instead of mirroring the structure here.
if snapshot.client_settings().is_language_services_disabled() { | ||
return Ok(None); | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried doing this centrally in background_request_task
but that didn't work:
diff --git a/crates/ty_server/src/server/api.rs b/crates/ty_server/src/server/api.rs
index a9dd9da1d0..9899184fd4 100644
--- a/crates/ty_server/src/server/api.rs
+++ b/crates/ty_server/src/server/api.rs
@@ -143,7 +143,18 @@ fn background_request_task<'a, R: traits::BackgroundDocumentRequestHandler>(
Box::new(move |notifier, responder| {
let _span = tracing::trace_span!("request", %id, method = R::METHOD).entered();
- let result = R::run_with_snapshot(&db, snapshot, notifier, params);
+ let result = if snapshot.client_settings().is_language_services_disabled()
+ && matches!(
+ R::METHOD,
+ request::GotoTypeDefinitionRequestHandler::METHOD
+ | request::CompletionRequestHandler::METHOD
+ | request::InlayHintRequestHandler::METHOD
+ | request::HoverRequestHandler::METHOD
+ ) {
+ // Return an empty response here ...
+ } else {
+ R::run_with_snapshot(&db, snapshot, notifier, params)
+ };
respond::<R>(id, result, &responder);
})
}))
But, then we'd need to somehow be able to get an "empty" response from those handlers which could be None
or something else.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I prefer having this in the specific handlers. It's less surprising, and someone copying an existing handler (which I always do as a starting point) is then also more likely to think about checking this option.
I guess, it's somewhat wasteful to still have the client send us requests but I suspect that unregistering the providers is more complicated and not supported by all clients?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess, it's somewhat wasteful to still have the client send us requests but I suspect that unregistering the providers is more complicated and not supported by all clients?
I think unregistering only works for capabilities that support dynamic registration and I'm not sure if all language capabilities supports that. And, this would also require that we go through the dynamic registration path (via registering each capabilities dynamically) to un-register the capability later as it requires the id that the server used to perform the registration.
f335a4d
to
f041dc0
Compare
CodSpeed Performance ReportMerging #18230 will not alter performanceComparing Summary
|
This PR is based on top of #18650 because it's just easier to not have to deal with the (now removed) experimental section. |
if snapshot.client_settings().is_language_services_disabled() { | ||
return Ok(None); | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I prefer having this in the specific handlers. It's less surprising, and someone copying an existing handler (which I always do as a starting point) is then also more likely to think about checking this option.
I guess, it's somewhat wasteful to still have the client send us requests but I suspect that unregistering the providers is more complicated and not supported by all clients?
f041dc0
to
e6cca67
Compare
## Summary PR adding support for it in the language server: astral-sh/ruff#18230 This PR adds support for `python.ty.disableLanguageServices` in the extension to extract the value and pass it as server setting. Closes: #20 ## Test Plan The video starts by the commented out `python.ty.disableLanguageServices` and you can see that there are inlay hints, hover support, diagnostics, etc. The hover content for `x` is `Literal[1]`. Once it's set to true, the server restarts and inlay hints are gone, hover content is gone but the diagnostics still remain as expected. https://github.com/user-attachments/assets/9280e7b0-bf89-4d99-bfa9-d10a6d3368f7
Summary
PR adding support for it in the VS Code extension: astral-sh/ty-vscode#36
This PR adds support for
python.ty.disableLanguageServices
to the ty language server by accepting this as server setting.This has the same issue as astral-sh/ty#282 in that it only works when configured globally. Fixing that requires support for multiple workspaces in the server itself.
I also went ahead and did a similar refactor as the Ruff server to use "Options" and "Settings" to keep the code consistent although the combine functionality doesn't exists yet because workspace settings isn't supported in the ty server.
Test Plan
Refer to astral-sh/ty-vscode#36 for the test demo.