-
Notifications
You must be signed in to change notification settings - Fork 11
Add python.languageServer as None configuration default
#283
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
Add python.languageServer as None configuration default
#283
Conversation
|
Thank you. I've to think about this a little more. It makes me uneasy to override settings from other extensions without the user's consent. On the other hand, it seems sort of nice because it does the right thing, unless the user explicitly selected a different language server. The other potential downside is that it is always applied, regardless of whether the user disabled ty's language server features. I remember that @dhruvmanila looked into this a while ago and I remember you had some ideas. Can you remind me of what they were? |
|
Yeah, the way I originally planned was to:
Pyrefly has done exactly this except that they have a small hack because there's a open issue on the Python VS Code extension (microsoft/vscode-python#25041). Overall, I don't think the approach implemented in this PR is correct as it'll always be applied (as noted by Micha) and we actually need to update both the ty and Python extension to make this correct. One thing to note is that I think I'd prefer to have |
|
Thanks for writing this down. I wonder if this PR is still a good first step. Users that ty's LSP are very likely users that also want to disable pylance LSP's. I'd be okay merging this PR but we should also update the documentation. |
Does this create an issue if a user wants to use ty for type checking but another language server for other services? So, will this always override the following to {
"ty.disableLanguageServices": true,
"python.languageServer": "Pylance"
} |
The way I understand it is that it only overrides the option if the user didn't explicitly configure the option. Setting |
|
I see. But, then there would be an issue when a user did not explicitly set |
Yes, but I consider this the less common use case. The reason I think this PR is an improvement is that it will "just work" for the majority of users and only a smaller subset needs to make changes to their configuration (only the few users that install ty because they want to use it for type checking but not for LSP features). |
|
We should specify this change in the README of this repository which can be done either in this PR or a follow-up PR. Additionally, I think we should also specify this in https://docs.astral.sh/ty/editors/#vs-code. |
|
Sounds good to me. @dhruvmanila coud you own the documentation change unless, @MeGaGiGaGon you're interested in contributing the changes. |
|
It would be nice if you could do it @dhruvmanila , I haven't dealt with the ty documentation at all and I heard some talk about needing to have ty compiled locally to make it work so it would be nice if you did it instead. |
python.languageServer as None configuration default
|
(I'll open a follow-up PR for documentation.) |
Summary
Fixes astral-sh/ty#2354
After doing a clean install of basedpyright and noticing the issue wasn't happening, I checked the value of
python.languageServerand noticed it wasNoneinstead of the usual defaultDefault, but only when I used basedpyright instead of ty.Searching in their repo for "python.languageServer", I saw they had this entry in their
package.json:While I'm not sure of what the rest of that does, I think the
"python.languageServer": "None"is what does the magic, so this PR adds that to ty'spackage.json.Test Plan
I preformed the same clean install steps from astral-sh/ty#2354 , but before I opened the python file I manually updated ty's package.json (in
data\extensions\astral-sh.ty-2026.2.0-win32-x64) and then observed the bug not happening even though I never touchedpython.languageServerin the VSC settings.