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

sort diagnostics and select the closest one on navigation #2273

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

rchl
Copy link
Member

@rchl rchl commented May 17, 2023

Show diagnostics in the quick panel in a sorted order and select the one closest to the selection on opening the quick panel.

We had discussions about that before but with time I came to conclusion that it's just more useful to have them sorted which helps with navigating to next/previous one. Especially combined with a behavior where the diagnostic closest to the selection is selected initially in the quick panel.

@jwortmann
Copy link
Member

So.... here's a little rant, but please don't take it personally :)

which helps with navigating to next/previous one

Navigating by location can be done with key bindings for the lsp_next_diagnostic and lsp_prev_diagnostic commands, where I forced the sorting by location. As far as I understand, when the diagnostics panel was introduced, it was meant as an explicit feature to not have the diagnostics sorted, and instead let the view jump around randomly when you press the down arrow key multiple times. I personally still find that weird, though. And at that time my PR to add a setting for this to at least allow users to configure the sort order could not get sufficient approval. Now you want to change the behavior without even providing a way to use the server-sort order? I guess then it will be your task to convince the voices that argued against this, see e.g. #1884 (comment).

What I don't really like, is that now diagnostics would be sorted by location in the "Goto Diagnostic" quick panel, but not in the diagnostics bottom panel. I'd rather prefer to have a consistent sort order everywhere where they are presented to the user, so I think that part of my implementation in #1909 was better. And it also gave the option to sort by severity.


Okay, regarding what the PR does, I would support this change and think it would be an improvement to the current behavior. And the idea to select the closest diagnostic instead of the first in the list is also a good one (at least when sorted by location).

@rchl
Copy link
Member Author

rchl commented May 18, 2023

Yeah, my opinion has changed over time. I thought that if we change this behavior then nobody will miss the previous one because frankly it feels random right now. Especially with multiple servers active. Please say how you feel about that now @jvican. I guess we can introduce a setting for controlling that, if there is disagreement.

I remembered we had discussion about that but I forgot that you've made a PR.

Navigating by location can be done with key bindings for the lsp_next_diagnostic and lsp_prev_diagnostic commands, where I forced the sorting by location

It's just not the same though. You don't know how many diagnostics there are if you are skipping just from one to the next. And it takes more time to orientate since you don't have an overview of the issues and their messages until you navigate to one.

But looking at your PR, it wouldn't work now because DiagnosticsStorage is per-session so it wouldn't work correctly with multiple sessions. I guess it was different when the PR was made and we had DiagnosticsManager.

What I don't really like, is that now diagnostics would be sorted by location in the "Goto Diagnostic" quick panel, but not in the diagnostics bottom panel.

We would have to support that too then.

@rchl rchl marked this pull request as draft June 24, 2023 07:49
@rchl rchl marked this pull request as ready for review October 23, 2023 20:32
@rchl
Copy link
Member Author

rchl commented Oct 23, 2023

I've updated the code to also sort diagnostics panel. Maybe went a bit too far with supporting asc/desc sort order since nothing is using "desc" order...

After making those changes I've also re-discovered #1909 which I'm still open to switching to instead. But maybe we can wait with introducing the setting until someone asks for it.

@rchl rchl changed the title sort diagnostics in quick panel and select closest one sort diagnostics and select the closest one on navigation Oct 23, 2023
Comment on lines 247 to +251
for i, session in enumerate(self.sessions):
for diagnostic in filter(is_severity_included(max_severity),
session.diagnostics.diagnostics_by_parsed_uri(self.parsed_uri)):
lines = diagnostic["message"].splitlines()
first_line = lines[0] if lines else ""
if len(lines) > 1:
first_line += " …"
text = "{}: {}".format(format_severity(diagnostic_severity(diagnostic)), first_line)
annotation = format_diagnostic_source_and_code(diagnostic)
kind = DIAGNOSTIC_KINDS[diagnostic_severity(diagnostic)]
list_items.append(sublime.ListInputItem(text, (i, diagnostic), annotation=annotation, kind=kind))
return list_items
for diagnostic in filter(is_severity_included(max_severity), session.diagnostics.diagnostics_by_parsed_uri(
self.parsed_uri, sort_order='asc')):
diagnostics.append((i, diagnostic))
selected_index = 0
Copy link
Member Author

Choose a reason for hiding this comment

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

No, this is only gonna sort per-session so not really how it should be...

Copy link
Member

@jwortmann jwortmann left a comment

Choose a reason for hiding this comment

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

I must say I don't really see a point in having the option for a descending sort order. If you want to scroll "backwards" through the diagnostics of a file, you could just select the last list item and then press the up arrow key repeatedly to scroll backwards through the diagnostics one by one. But I don't know why anyone would want to do that ;-)

If we don't want to introduce a new setting for the sorting method (position / severity / unsorted), then maybe we could at least add it as another command argument for the LspGotoDiagnosticCommand. Then, if desired, users could customize the menu and command palette entry with a different sorting method, if they don't like the default one. This would mean that it's only configurable for the quick panel, and not for the bottom panel, but maybe it would be an acceptable solution for users who would like to keep the current (unsorted) ordering.

@@ -221,10 +228,11 @@ def _project_path(self, parsed_uri: ParsedUri) -> str:
return path


class DiagnosticInputHandler(sublime_plugin.ListInputHandler):
class DiagnosticInputHandler(PreselectedListInputHandler):
Copy link
Member

Choose a reason for hiding this comment

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

This should not be a PreselectedListInputHandler. I mean it wouldn't break anything, but it also makes no sense if it's always gonna be instantiated without a initial value to preselect, so it would just be confusing here. PreselectedListInputHandler only makes sense if it's not the last InputHandler on the stack.

Comment on lines +38 to +39
SessionIndex = int
SelectedIndex = int
Copy link
Member

Choose a reason for hiding this comment

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

Hm, do you really want to add type aliases for such basic types like int? I would think that it makes the code more difficult to follow, rather than easier, because when you see SelectedIndex you might not remember whether it's a special class, or or a TypedDict or something else. Then you first need to look up the definition to see what it is.
And where would we stop with it; should every int and str get a special name to describe what it is, for example session_name: SessionName etc. ...

I'm aware that URI and DocumentUri exist as aliases for str in the protocol, but they have some kind of special meaning, namely it guarantees that the contents of those strings can be parsed as a valid URI.

Copy link
Member

Choose a reason for hiding this comment

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

type aliases for such basic types like int?

For me it makes the type more explicit and easier to follow>

If we used int

ListItemsReturn = Union[List[str], Tuple[List[str], int],

Instead of SelectedIndex

ListItemsReturn = Union[List[str], Tuple[List[str], SelectedIndex],

I would not know in the first example if int should represent a SelectedIndex or a SessionIndex, or something else.

Copy link
Member

Choose a reason for hiding this comment

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

Maybe then we should add docstrings to type stubs used by LSP?

Compare (missing) info from the type stubs in this repository

lsp

with the stubs shipped by LSP-pyright:

pyright

Otherwise, where would we stop? Should all and every int used in LSP get a type alias, or do we need to define certain rules for that beforehand...?

Copy link
Member

@jwortmann jwortmann Oct 24, 2023

Choose a reason for hiding this comment

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

Instead of storing the session index as an int, I would probably recommend to refactor that code anyways and use the session name instead. This code session = self.sessions[i] which is used in the DiagnosticInputHandler class looks quite fragile to me (is it safe to still work correctly even when one of the sessions is closed or a new server is started in the meanwhile?)

Edit: I see the sessions are fixed when the InputHandler instance gets created, so I guess it's safe to use here. (I was thinking of the self.sessions() from the LSP commands)

@rchl
Copy link
Member Author

rchl commented Oct 24, 2023

I must say I don't really see a point in having the option for a descending sort order.

I remember now that I've added the sort order because I was planning to use it in place of this line also:

diagnostics.sort(key=lambda d: operator.itemgetter('line', 'character')(d['range']['start']), reverse=not forward)

But I did not replace it when I've realized that those need to be sorted after collecting diagnostics from all sessions. So yeah, currently having support for descending sort order makes no sense but depending on how I'll handle this issue with sorting, maybe it will.

@jvican
Copy link

jvican commented Oct 29, 2023

Regarding you previous question @rchl, I'm willing to give this one a try and see how it does with the LSP servers I use. If it turns out that the diagnostics order is significantly worsened for me (especially in multi-module projects where sorting doesn't take into account the build order in which projects need to be built), I'm happy to consider making a PR to add more ways of configuring the sorting, or make it possible to opt-out of it.

@rchl rchl marked this pull request as draft October 31, 2023 17:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants