Skip to content
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

Situational LSP on same file extension #8505

Closed
ngasull opened this issue Oct 10, 2023 · 5 comments · Fixed by #8696
Closed

Situational LSP on same file extension #8505

ngasull opened this issue Oct 10, 2023 · 5 comments · Fixed by #8696
Labels
A-helix-term Area: Helix term improvements C-enhancement Category: Improvements E-easy Call for participation: Experience needed to fix: Easy / not much E-good-first-issue Call for participation: Issues suitable for new contributors

Comments

@ngasull
Copy link

ngasull commented Oct 10, 2023

Intro

Some languages have multiple possible language servers, in my case the (in)famous Javascript/Typescript:

  • typescript-language-server
  • deno lsp
  • even eslint, tailwindcss or any other situational language support, all for the same file types (jsx?/tsx?)

Formal issue

Language support may depend on configured tooling in the workspace rather than just file extension. roots isn't requiring the existence of the files.

Possible enhancements

  1. Adding a language.toml flag key to actually require one of the roots files to be found to in order to enable the LSP
    Eg. require-root = true
  2. Adding a language.toml configuration key of possible marker files in found root that enables the LSP.
    Eg. required-roots = ["deno.json", "deno.jsonc", "deno.lock"]

Option 1 seems relevant and sufficient to cover multiple LSP per file type while keeping things simple.

Current workaround

Dropping an .helix/languages.toml situationally in workspaces.

@ngasull ngasull added the C-enhancement Category: Improvements label Oct 10, 2023
@kirawi kirawi added E-easy Call for participation: Experience needed to fix: Easy / not much A-helix-term Area: Helix term improvements E-good-first-issue Call for participation: Issues suitable for new contributors labels Oct 15, 2023
@ontley
Copy link
Contributor

ontley commented Oct 26, 2023

I want to work on this, but I don't really know where this would check be performed? Registry::get maybe?

@pascalkuthe
Copy link
Member

yeah registry::get is mostly the right place.

I don't think the option should be to only start if roots exists but a bit more general: to only start if the detected lsp root directory (there is always one even if none of the root markers are found) contains files matching any of a list of globs (use globset since we already depend on it).

That way you could also check for a different file here and copying the root markers isn't too hard.

The best way to implement this is probably to make start_client return an Result<Option<T>> (None corresponds to no marker found and you can simply use the ? with filter_map to filter those kinds of servers

@ontley
Copy link
Contributor

ontley commented Oct 27, 2023

Makes sense, I'll see what I can do.

Unfortunately I'm away until next thursday.

@ontley
Copy link
Contributor

ontley commented Nov 1, 2023

the detected lsp root directory (there is always one even if none of the root markers are found)

Does this refer to Config.workspace_lsp_roots / the root_dirs argument in start_client? They seem to be empty even after :lsp-restart

@ontley
Copy link
Contributor

ontley commented Nov 2, 2023

Nvm, I realized I need root_path from Client::start

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-helix-term Area: Helix term improvements C-enhancement Category: Improvements E-easy Call for participation: Experience needed to fix: Easy / not much E-good-first-issue Call for participation: Issues suitable for new contributors
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants