Add support for showing all LSPs in --health #7315

TheRealLorenz · 2023-06-11T17:17:27Z

Hi there, today I had to debug an lsp configuration with multiple servers and came across this missing feature.
I wrote a quick implementation for the --health <language> subcommand, but now I'm stuck at --health languages.

In my machine --health languages doesn't fully show lang-servers' names and if I were to print all names one after another it would be pretty useless.

My idea was to check for the number of lang-servers and if there are more than one than the output could be something like (the names are pretty bad, I'm seeking better ideas):

✓ All (in green, it means all servers are ok)
- Some (in yellow, it means some servers are ok and some are not)
✘ None (in red, it means all servers are not ok)

Then for a more detailed output the user could use --health <language>.

Otherwise we could show every lang-server name in a different line, but I think it could become a little cumbersome to implement.

Let me know your thoughts!

gabydd · 2023-06-12T13:10:03Z

Maybe it would be better then "all, some, none" to represent it will the symbol plus <amount of available servers>/<amount of configured servers>

TheRealLorenz · 2023-06-12T17:47:54Z

Update

As @gabydd suggested, I updated the code and now --health languages looks like this:

✓ 2/2 (in green, it means all servers are ok)
- 1/3 (in yellow, it means some servers are ok and some are not)
✘ 0/4 (in red, it means all servers are not ok)

I'm still not sure about the symbol for the yellow message, I don't like the hyphen very much. Maybe I could just use ✓ or ✘ ?

helix-term/src/health.rs

TheRealLorenz · 2023-06-14T08:26:39Z

Should I also update the docs/wiki?

CptPotato · 2023-06-15T12:29:02Z

I'm still not sure about the symbol for the yellow message, I don't like the hyphen very much. Maybe I could just use ✓ or ✘ ?

The way I understand it, multiple language servers for a language are usually alternatives, right?
In that case I think a check mark ✓ is fine.

Edit: Ah, thanks for the clarification @AlexanderBrevig

AlexanderBrevig · 2023-06-15T12:47:52Z

The way I understand it, multiple language servers for a language are usually alternatives, right? In that case I think a check mark ✓ is fine.

They all run simultaneously.
A typical setup would be ["typescript", "eslint", "prettier"] for a typescript setup.
Missing one of them would not feel ✓ and as such, I vote for the - indicating partial setup.

TheRealLorenz · 2023-06-15T12:50:36Z

Perfect. I'm updating the wiki as soon as this pr gets merged. I haven't find any mention of the healthchecks inside the docs.

gyreas · 2023-06-20T10:23:36Z

Will it be possible to specify the languages to display as such: hx --health lang1 lang2...langn?

TheRealLorenz · 2023-06-20T10:26:22Z

Will it be possible to specify the languages to display as such: hx --health lang1 lang2...langn?

Well I don't think it would be difficult to implement, but I think it should be done in a different PR.

Maybe if you open an issue, I could start to work on that!

Better output (inspired by helix-editor#8156). Handle the case where no LSPs are configured.

TheRealLorenz · 2023-09-03T18:34:11Z

Current output of `--health` with this PR

--health languages:

css            ✓ vscode-css-language-server (green)
svelte         ✘ 0/3 (red)
tsx            - 2/3 (yellow)
typescript     ✓ 2/2 (green)
yuck           None (yellow)
zig            ✘ zls (red)

--health <language>: (symbols are green or red, paths are green, errors are red and "None" is yellow)

Configured language servers:
  ✓ typescript-language-server: /Users/lollo/Library/pnpm/typescript-language-server
  ✓ vscode-eslint-language-server: /Users/lollo/Library/pnpm/vscode-eslint-language-server
Configured debug adapter: None
Highlight queries: ✓
Textobject queries: ✓
Indent queries: ✓

Configured language servers:
  ✓ clangd: /usr/bin/clangd
Configured debug adapter: lldb-vscode
Binary for debug adapter: 'lldb-vscode' not found in $PATH
Highlight queries: ✓
Textobject queries: ✓
Indent queries: ✓

Configured language servers: None
Configured debug adapter: None
Highlight queries: ✓
Textobject queries: ✘
Indent queries: ✘

archseer · 2023-09-07T13:59:42Z

svelte ✘ 0/3 (red)
tsx - 2/3 (yellow)
typescript ✓ 2/2 (green)

I'm not sure this is the best UI, it makes it look like I need to install all the servers where one should be enough. I'd maybe list the ones that were detected and then a number of those that weren't found.

TheRealLorenz · 2023-09-07T16:54:57Z

svelte ✘ 0/3 (red)
tsx - 2/3 (yellow)
typescript ✓ 2/2 (green)

I'm not sure this is the best UI, it makes it look like I need to install all the servers where one should be enough. I'd maybe list the ones that were detected and then a number of those that weren't found.

My first idea was actually to make something like:

css            ✓ vscode-css-language-server (green)
svelte         ✘ svelteserver (red)
               ✘ eslint-language-server (red)
               ✘ emmet-langugage-server (red)
tsx            ✓ typescript-language-server (green)
               ✘ eslint-language-server (red)
               ✘ emmet-langugage-server (red)
typescript     ✓ typescript-language-server (green)
               ✘ eslint-language-server (red)
yuck           None (yellow)
zig            ✘ zls (red)

But I though it would have been a little cumbersome to implement.

Let me know if this may be a better solution to the problem.

Displays all LSPs as a list in the table generated wih `--health languages`

Avoids some calls to .clone()

EpocSquadron · 2023-09-07T20:31:55Z

I'm not sure this is the best UI, it makes it look like I need to install all the servers where one should be enough.

Technically, if there are multiple lsps configured this is exactly the case (I configured two more, now I have to see if helix sees them). That is unless and until multiple lsps are configured by default for a language specifically to provide fallbacks rather than expecting multiple to be required. In that case, the alternate UX @TheRealLorenz and I independently suggested is better.

TheRealLorenz · 2023-09-08T17:27:02Z

I think this PR should be included in the upcoming release, as it will be the first release with multiple LSP support.

TheRealLorenz · 2023-09-25T20:42:59Z

svelte ✘ 0/3 (red)
tsx - 2/3 (yellow)
typescript ✓ 2/2 (green)

I'm not sure this is the best UI, it makes it look like I need to install all the servers where one should be enough. I'd maybe list the ones that were detected and then a number of those that weren't found.

My first idea was actually to make something like:
css            ✓ vscode-css-language-server (green)
svelte         ✘ svelteserver (red)
               ✘ eslint-language-server (red)
               ✘ emmet-langugage-server (red)
tsx            ✓ typescript-language-server (green)
               ✘ eslint-language-server (red)
               ✘ emmet-langugage-server (red)
typescript     ✓ typescript-language-server (green)
               ✘ eslint-language-server (red)
yuck           None (yellow)
zig            ✘ zls (red)
But I though it would have been a little cumbersome to implement.

Let me know if this may be a better solution to the problem.

I've already implemented this (need to push it), I'm just looking for a feedback now

helix-term/src/health.rs

the-mikedavis · 2023-10-06T20:21:30Z

That alternate UI looks clearer to me

TheRealLorenz · 2023-10-07T16:46:39Z

Pushed commits with the alternative UI and applied the requested changes!

the-mikedavis

Just a small style nit. I'm liking the way this is looking now 👍

helix-term/src/health.rs

David-Else · 2023-10-11T16:19:20Z

I tested this, all seems great!

Please try to merge it before the big new release as it will be very important to problem shoot the new multiple language server feature.

@AlexanderBrevig

* Add support for showing all LSPs in --health <lang> * Add support for showing all LSPs in --health languages * Use available/configured in --health languages * Apply @AlexanderBrevig suggestion in --health * Update `--health <language>` Better output (inspired by helix-editor#8156). Handle the case where no LSPs are configured. * Display all LSPs in `--health languages` instead of x/x Displays all LSPs as a list in the table generated wih `--health languages` * Make check_binary accept Optional references to str Avoids some calls to .clone() * Apply @the-mikedavis suggestions * Avoid useless collecting and cloning * Use for loop instead of .try_for_each()

@AlexanderBrevig

* Add support for showing all LSPs in --health <lang> * Add support for showing all LSPs in --health languages * Use available/configured in --health languages * Apply @AlexanderBrevig suggestion in --health * Update `--health <language>` Better output (inspired by helix-editor#8156). Handle the case where no LSPs are configured. * Display all LSPs in `--health languages` instead of x/x Displays all LSPs as a list in the table generated wih `--health languages` * Make check_binary accept Optional references to str Avoids some calls to .clone() * Apply @the-mikedavis suggestions * Avoid useless collecting and cloning * Use for loop instead of .try_for_each()

@AlexanderBrevig

* Add support for showing all LSPs in --health <lang> * Add support for showing all LSPs in --health languages * Use available/configured in --health languages * Apply @AlexanderBrevig suggestion in --health * Update `--health <language>` Better output (inspired by helix-editor#8156). Handle the case where no LSPs are configured. * Display all LSPs in `--health languages` instead of x/x Displays all LSPs as a list in the table generated wih `--health languages` * Make check_binary accept Optional references to str Avoids some calls to .clone() * Apply @the-mikedavis suggestions * Avoid useless collecting and cloning * Use for loop instead of .try_for_each()

@AlexanderBrevig

* Add support for showing all LSPs in --health <lang> * Add support for showing all LSPs in --health languages * Use available/configured in --health languages * Apply @AlexanderBrevig suggestion in --health * Update `--health <language>` Better output (inspired by helix-editor#8156). Handle the case where no LSPs are configured. * Display all LSPs in `--health languages` instead of x/x Displays all LSPs as a list in the table generated wih `--health languages` * Make check_binary accept Optional references to str Avoids some calls to .clone() * Apply @the-mikedavis suggestions * Avoid useless collecting and cloning * Use for loop instead of .try_for_each()

TheRealLorenz added 2 commits June 11, 2023 19:07

Add support for showing all LSPs in --health <lang>

1f7669b

Add support for showing all LSPs in --health languages

5377947

Use available/configured in --health languages

fa6e386

AlexanderBrevig reviewed Jun 14, 2023

View reviewed changes

helix-term/src/health.rs Outdated Show resolved Hide resolved

Apply @AlexanderBrevig suggestion in --health

379691c

TheRealLorenz marked this pull request as ready for review June 14, 2023 08:26

kirawi added A-helix-term Area: Helix term improvements S-waiting-on-review Status: Awaiting review from a maintainer. labels Jun 15, 2023

TheRealLorenz mentioned this pull request Jun 20, 2023

Specify the languages to display in --health as such: hx --health lang1 lang2...langn #7401

Closed

gabydd mentioned this pull request Aug 4, 2023

feat(health): show info about multiple lsp/dap binaries #7833

Closed

kirawi mentioned this pull request Sep 3, 2023

Health check subcommand doesn't display multiple language servers #8156

Closed

Update --health <language>

8fc0bc1

Better output (inspired by helix-editor#8156). Handle the case where no LSPs are configured.

TheRealLorenz added 2 commits September 7, 2023 19:51

Display all LSPs in --health languages instead of x/x

cf729c5

Displays all LSPs as a list in the table generated wih `--health languages`

Make check_binary accept Optional references to str

657695b

Avoids some calls to .clone()

the-mikedavis reviewed Oct 6, 2023

View reviewed changes

helix-term/src/health.rs Outdated Show resolved Hide resolved

helix-term/src/health.rs Outdated Show resolved Hide resolved

Apply @the-mikedavis suggestions

753e55a

Avoid useless collecting and cloning

d9647d3

the-mikedavis reviewed Oct 9, 2023

View reviewed changes

helix-term/src/health.rs Outdated Show resolved Hide resolved

Use for loop instead of .try_for_each()

45e66c5

the-mikedavis linked an issue Oct 16, 2023 that may be closed by this pull request

Health check subcommand doesn't display multiple language servers #8156

Closed

the-mikedavis approved these changes Oct 16, 2023

View reviewed changes

the-mikedavis added this to the 23.10 milestone Oct 16, 2023

pascalkuthe approved these changes Oct 16, 2023

View reviewed changes

pascalkuthe merged commit d9d7f67 into helix-editor:master Oct 16, 2023
6 checks passed

TheRealLorenz deleted the master branch October 18, 2023 08:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for showing all LSPs in --health #7315

Add support for showing all LSPs in --health #7315

TheRealLorenz commented Jun 11, 2023 •

edited

Loading

gabydd commented Jun 12, 2023

TheRealLorenz commented Jun 12, 2023 •

edited

Loading

TheRealLorenz commented Jun 14, 2023

CptPotato commented Jun 15, 2023 •

edited

Loading

AlexanderBrevig commented Jun 15, 2023

TheRealLorenz commented Jun 15, 2023

gyreas commented Jun 20, 2023

TheRealLorenz commented Jun 20, 2023 •

edited

Loading

TheRealLorenz commented Sep 3, 2023 •

edited

Loading

archseer commented Sep 7, 2023

TheRealLorenz commented Sep 7, 2023 •

edited

Loading

EpocSquadron commented Sep 7, 2023

TheRealLorenz commented Sep 8, 2023

TheRealLorenz commented Sep 25, 2023

the-mikedavis commented Oct 6, 2023

TheRealLorenz commented Oct 7, 2023

the-mikedavis left a comment

David-Else commented Oct 11, 2023

Add support for showing all LSPs in --health #7315

Add support for showing all LSPs in --health #7315

Conversation

TheRealLorenz commented Jun 11, 2023 • edited Loading

gabydd commented Jun 12, 2023

TheRealLorenz commented Jun 12, 2023 • edited Loading

Update

TheRealLorenz commented Jun 14, 2023

CptPotato commented Jun 15, 2023 • edited Loading

AlexanderBrevig commented Jun 15, 2023

TheRealLorenz commented Jun 15, 2023

gyreas commented Jun 20, 2023

TheRealLorenz commented Jun 20, 2023 • edited Loading

TheRealLorenz commented Sep 3, 2023 • edited Loading

Current output of --health with this PR

archseer commented Sep 7, 2023

TheRealLorenz commented Sep 7, 2023 • edited Loading

EpocSquadron commented Sep 7, 2023

TheRealLorenz commented Sep 8, 2023

TheRealLorenz commented Sep 25, 2023

the-mikedavis commented Oct 6, 2023

TheRealLorenz commented Oct 7, 2023

the-mikedavis left a comment

Choose a reason for hiding this comment

David-Else commented Oct 11, 2023

TheRealLorenz commented Jun 11, 2023 •

edited

Loading

TheRealLorenz commented Jun 12, 2023 •

edited

Loading

CptPotato commented Jun 15, 2023 •

edited

Loading

TheRealLorenz commented Jun 20, 2023 •

edited

Loading

TheRealLorenz commented Sep 3, 2023 •

edited

Loading

Current output of `--health` with this PR

TheRealLorenz commented Sep 7, 2023 •

edited

Loading