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

Adds a diagnostics severity marker to diagnostics picker #5110

Conversation

zummenix
Copy link
Contributor

Diagnostics severity markers E, W, I, H in diagnostics picker.

As noted in the related issue comments, this allows filtering inside the picker by these markers.
This is also useful for people who have color blindness.

Screenshot 2022-12-10 at 17 52 04

Screenshot 2022-12-10 at 17 52 31

Screenshot 2022-12-10 at 17 53 20

I was trying different formats and styles. Format like [E] looks heavy and takes up a precious space. Styling markers with diagnostics color looks a little weird, especially with a file path in workspace diagnostics picker. In any case I'm open to any suggestions.

Closes #3543

helix-term/src/commands/lsp.rs Outdated Show resolved Hide resolved
_ => ' ',
})
.unwrap_or(' ');
format!("{} ", symbol)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm a little sad we need to allocate all the time for each and every diagnostic.

If Span::raw accept &'static str, we could simply embed the space in the returned string and do something like:

            // Two characters wide, so two spaces in the default
            .map_or("  ", |s| match s {
                DiagnosticSeverity::HINT => "H ",
                DiagnosticSeverity::INFORMATION => "I ",
                DiagnosticSeverity::WARNING => "W ",
                DiagnosticSeverity::ERROR => "E ",
                _ => "  ",
            })

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As long as not config option is offered for this, it would work well

Copy link
Member

@pascalkuthe pascalkuthe Dec 10, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A Span is a Cow so it is possible to create one from a &'static str without allocating so I also think we should return a str and avoid allocation.. Even if this was configurable it should be possible to use a reference to the config here

@zummenix zummenix force-pushed the severity-marker-in-diagnostics-picker branch from 8bc44b8 to 8e1b780 Compare December 10, 2022 16:17
@zummenix zummenix requested review from poliorcetics and pascalkuthe and removed request for poliorcetics and pascalkuthe December 10, 2022 16:18
@CptPotato
Copy link
Contributor

FYI, this somewhat overlaps with icon support: #2869 (comment)

@pascalkuthe
Copy link
Member

FYI, this somewhat overlaps with icon support: #2869 (comment)

Definitely similar thanks for providing additional context.
The critical difference between those two PRs is that the characters used in this PR can (quickly) typed and used to filter by diagnostic type (with #5114) while the icons are mostly just additional eye-candy. I think the colored text already makes it quite clear what kind of diagnostic I am looking at however I find the additional filtering offered by this PR quite useful.

For people that prefer an icon here I think a simple config option to change the displayed text could be added in the future, although that would disable filtering (there is an option to add different text for filtering then is being displayed but using this for anything but a performance optimization would not only be confusing but also seems to break highlighting of matched incidences).

Copy link
Member

@pascalkuthe pascalkuthe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍 I like this change. It's a useful way to filter diagnostics that doesn't impede existing use-cases and integrates well with the picker.

@pascalkuthe pascalkuthe added C-enhancement Category: Improvements A-language-server Area: Language server client A-helix-term Area: Helix term improvements S-waiting-on-review Status: Awaiting review from a maintainer. labels Dec 11, 2022
@poliorcetics
Copy link
Contributor

poliorcetics commented Feb 16, 2023

This has been sitting approved for two months now and it's a fairly small change, are there any blockers ?

@pascalkuthe
Copy link
Member

pascalkuthe commented Feb 16, 2023

This has been sitting approved for two months now and it's a fairly small change, are there any blockers ?

There are loads of PRs that sit approved for a while. The review bandwidth of @archseer and @the-mikedavis is limited. We recive and merge a huge number of PRs. Eventually this PR will be reviewed by them.

@zummenix zummenix force-pushed the severity-marker-in-diagnostics-picker branch from 2f2e6cb to e7ef955 Compare November 26, 2023 14:17
Copy link
Member

@the-mikedavis the-mikedavis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Eventually we want this and similar info (for example symbol kind in the symbol picker) in the pickers but just concatenating it into the row seems like a bandaid to me. I'd rather see the picker organized as a table so this info can be a column - it's included in the linked table picker PR.

@zummenix
Copy link
Contributor Author

Closing in favor of #7265

@zummenix zummenix closed this Nov 27, 2023
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 A-language-server Area: Language server client C-enhancement Category: Improvements S-waiting-on-review Status: Awaiting review from a maintainer.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Sort diagnostics by gravity (error -> warning -> info)
5 participants