-
Notifications
You must be signed in to change notification settings - Fork 1.5k
[ty] Stabilize completions #18650
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
[ty] Stabilize completions #18650
Conversation
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
|
I think @dhruvmanila expressed some concerns around stabilizing the feature. We should make sure that he reviewed this PR before merging. |
/// This is a direct representation of the settings schema sent by the client. | ||
#[derive(Debug, Deserialize, Default)] | ||
#[cfg_attr(test, derive(PartialEq, Eq))] | ||
#[serde(rename_all = "camelCase")] | ||
pub struct ClientSettings { | ||
pub(crate) experimental: Option<Experimental>, |
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.
This is, technically, a breaking change. I think it's fine, considering where we're at with ty.
We'll also need to update the ty-vscode
extension to no longer list this option and update the documentation as well.
At this stage, the ty language server doesn't have all the capabilities that a developer might use on a daily basis e.g., go to definition, rename, etc. This means that it's more than likely that the ty language server will be used alongside another language server (e.g., Pyright) to provide other capabilities. Now, if we stabilize this i.e., show completions without opt-in, it would mean that the completion menu would contain duplicate entries from both the language server. One way to solve this, and is a solution that Pyrefly has implemented, would be to provide a The above solution is not granular as it would either enable all language features or disable them, nothing in between. Another solution would be to provide granular configuration, like allowing users to use completions from ty but navigation capabilities like goto definition, declaration, etc. from another language server. Or, an even granular options that allows users to enable / disable each capability. tl;dr The main issue here is how do we allow users to keep using ty server's capabilities even when another language server is active. I'd prefer if we can wait until we have the That said, I think we could still merge the PRs for adding the above config option on ty's side so that there's still an option for users to opt-out of language services like auto-completions. |
Hmmm okay. Do we already have a problem today with conflicting LSPs running simultaneously? e.g., If you do a goto type definition request with ty and pyright enabled, which one services it? Another issue I see is that even if we offer granular enable/disable options, that still seemingly requires the other LSP you use to also have granular enable/disable options. I'm fine waiting here for |
I think what we can do is:
Pyrefly has a hack that does the refresh on their VS Code extension. I'd prefer and see if I can just fix the issue on the Python extension side. I just didn't prioritized it as it wasn't that helpful earlier but given that completions would be available it might be useful to prioritize that now. |
So, all in all, I'm fine moving ahead with this PR. We should also remove this option from https://github.com/astral-sh/ty/blob/main/docs/reference/editor-settings.md#experimental. |
I think this depends on client implementation. For example, Neovim supports multiple servers for navigation capabilities like goto type definition where it'll collect all the locations for the request from all the available language servers and provide them as quickfix list so you can navigate through all available options. |
This was only used to opt into completions. But we're removing the opt-in in astral-sh/ruff#18650, so let's remove it from the docs too. Ref #86
This was only used to opt into completions. But we're removing the opt-in in astral-sh/ruff#18650, so let's remove it from the docs too. Ref #86
* main: (38 commits) [`pyupgrade`] Suppress `UP008` diagnostic if `super` symbol is not builtin (#18688) [pylint] Fix `PLW0128` to check assignment targets in square brackets and after asterisks (#18665) [`refurb`] Make the fix for `FURB163` unsafe for `log2`, `log10`, `*args`, and deleted comments (#18645) [ty] allow `T: Never` as subtype of `Never` (#18687) [ty] Use more parallelism when running corpus tests (#18711) [ty] Support `dataclasses.KW_ONLY` (#18677) [`ruff`] Check for non-context-manager use of `pytest.raises`, `pytest.warns`, and `pytest.deprecated_call` (`RUF061`) (#17368) Add syntax error when conversion flag does not immediately follow exclamation mark (#18706) [`flake8-pyi`] Fix `custom-typevar-for-self` with string annotations (`PYI019`) (#18311) Drop confusing second `*` from glob pattern example (#18709) [ty] Stabilize completions (#18650) [ty] Correctly label typeshed-sync PRs (#18702) Update Rust crate memchr to v2.7.5 (#18696) Update dependency react-resizable-panels to v3.0.3 (#18691) Update Rust crate clap to v4.5.40 (#18692) Update Rust crate libcst to v1.8.2 (#18695) Update Rust crate jiff to v0.2.15 (#18693) Update Rust crate libc to v0.2.173 (#18694) Update Rust crate syn to v2.0.103 (#18698) Update Rust crate toml to v0.8.23 (#18699) ...
## Summary Following up on astral-sh/ruff#18650, this PR removes the `experimental.completions.enable` config from the extension.
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:
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