sort diagnostics and select the closest one on navigation #2273
Conversation
|
So.... here's a little rant, but please don't take it personally :)
Navigating by location can be done with key bindings for the 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). |
|
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.
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
We would have to support that too then. |
|
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
changed the title
| 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 |
There was a problem hiding this comment.
No, this is only gonna sort per-session so not really how it should be...
There was a problem hiding this comment.
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): | |||
There was a problem hiding this comment.
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.
| SessionIndex = int | ||
| SelectedIndex = int |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Maybe then we should add docstrings to type stubs used by LSP?
Compare (missing) info from the type stubs in this repository
with the stubs shipped by LSP-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...?
There was a problem hiding this comment.
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)
I remember now that I've added the sort order because I was planning to use it in place of this line also: Line 293 in 757908a 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. |
|
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. |
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.