-
Notifications
You must be signed in to change notification settings - Fork 1.5k
Make completions an opt-in LSP feature #17921
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
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.
Thank you
crate::version(), | ||
)?; | ||
|
||
crate::message::init_messenger(connection.make_sender()); |
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 think it's important that init_logging
(line 53) happens after init_messenger
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.
Yeah, I think so too. The messenger needs to be initialized first because it's being used when serializing the initialization options. If there are invalid initialization options, it requires the messenger to provide a notification to the user.
|
||
impl Experimental { | ||
/// Returns `true` if completions are enabled in the settings. | ||
pub(crate) fn enable_completions(&self) -> 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.
nit: is_completions_enabled
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.
Nice!
05fbcbc
to
ba70a97
Compare
|
…lass * origin/main: [`pylint`] add fix safety section (`PLC2801`) (#17825) Add instructions on how to upgrade to a newer Rust version (#17928) [parser] Flag single unparenthesized generator expr with trailing comma in arguments. (#17893) [ty] Ensure that `T` is disjoint from `~T` even when `T` is a TypeVar (#17922) [ty] Sort collected diagnostics before snapshotting them in mdtest (#17926) [ty] Add basic file watching to server (#17912) Make completions an opt-in LSP feature (#17921) Add link to `ty` issue tracker (#17924) [ty] Add support for `__all__` (#17856) [ty] fix assigning a typevar to a union with itself (#17910) [ty] Improve UX for `[duplicate-base]` diagnostics (#17914) Clean up some Ruff references in the ty server (#17920)
Specifically, this PR reverts "Make completions an opt-in LSP feature (#17921)", corresponding to commit 51e2eff. In practice, this means you don't need to opt into completions working by enabling experimental features. i.e., I was able to remove this from my LSP configuration: ``` "experimental": { "completions": { "enable": true } }, ``` There's still a lot of work left to do to make completions awesome, but I think it's in a state where it would be useful to get real user feedback. It's also meaningfully using ty to provide completions that use type information. Ref astral-sh/ty#86
Specifically, this PR reverts "Make completions an opt-in LSP feature (#17921)", corresponding to commit 51e2eff. In practice, this means you don't need to opt into completions working by enabling experimental features. i.e., I was able to remove this from my LSP configuration: ``` "experimental": { "completions": { "enable": true } }, ``` There's still a lot of work left to do to make completions awesome, but I think it's in a state where it would be useful to get real user feedback. It's also meaningfully using ty to provide completions that use type information. Ref astral-sh/ty#86
Summary
We now expect the client to send initialization options to opt-in to experimental (but LSP-standardized) features, like completion support. Specifically, the client should set
"experimental.completions.enable": true
.Closes astral-sh/ty#74.