-
Notifications
You must be signed in to change notification settings - Fork 3.8k
feat: add experimental support for Ty language server #5575
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
- Introduced OPENCODE_EXPERIMENTAL_LSP_TY flag to conditionally enable Ty language server functionality. - Implemented Ty language server with support for Python file extensions and virtual environment detection.
…DE_EXPERIMENTAL_LSP_TY flag cleaner pattern - Added a function to conditionally enable or disable LSP servers (pyright and ty) based on the experimental flag.
|
/review |
| }, | ||
| experimental: true, | ||
| } | ||
|
|
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.
Potential issue: The experimental: true property is added here but it is not actually used anywhere. The filterExperimentalServers function checks for specific server IDs ("ty", "pyright") rather than using this property. This could be intentional for now, but it is worth noting that this property is currently unused.
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.
Makes sense bro-chacho
|
the only comment left that I recommend is killing that "experimental" key, ignore all the other agent feedback |
…y-lsp' into feat/experimental-ty-lsp
| id: "ty", | ||
| extensions: [".py", ".pyi"], | ||
| root: NearestRoot(["pyproject.toml", "ty.toml", "setup.py", "setup.cfg", "requirements.txt", "Pipfile", "pyrightconfig.json"]), | ||
| async spawn(root) { |
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.
isn't this spawning even when the var isn't set?
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.
Ahh I thought the way the logic goes, the servers only get spawned at after being loaded to the Instance.state? Before then the server is removed when the var isn't set.
But then again i could just add a check there just to be 1000% guaranteed.
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.
Just like how in index.ts
if (cfg.lsp === false) {
log.info("all LSPs are disabled")
return {
broken: new Set<string>(),
servers,
clients,
spawning: new Map<string, Promise<LSPClient.Info | undefined>>(),
}
}it can return just empty servers and spawning methods
Proposed fix for #5345