"Goto Diagnostic" quick panels. #1884
Conversation
* added quick panel commands: - lsp_goto_diagnostic - lsp_goto_diagnostic_for_document_uri - lsp_goto_diagnostic_in_project * made LocationPicker more general (new signature) * added SimpleLocationPicker wrapper (old signature) * DiagnosticsManager: - simplified and more general - added diagnostics_by_document_uri() - added diagnostics_by_parsed_uri() * added unparse_uri() to undo parse_uri()
|
Looks cool. Just one question. Why are errors not red? :) Also, I see this on
Referring to weird extra space. Possibly an ST bug. |
They aren't in my color scheme, took it from my screen, wasn't aware that it is from my color scheme, thought it was from LSP and found it cool. Okay, should default to red.
Something similar I also see. On my machine ST takes the size of the highest item and applies it to all others, also smaller ones. But on your screenshot is more extra space than I see here. I have build 4121 / Mac. Regarding the failing tests, my wild guess would be that with the location picker wrapper I broke mocking somewhere in the test suite. If not that, I have no clue. I'm afraid I won't be able to look into this. |
Note that I've changed it to red before making the screenshot so that might have been confusing. |
|
It perhaps would look better to use a ListInputHandler instead of the quick panel, because it's newer and would allow more styling, i.e. the long diagnostic messages could be put into the "details" panel at the bottom then. This might solve the extra space issue. Also note that this PR requires a new tag prefix Edit: The color kind ids for the quick panel require ST 4095 too. "Error" should be KIND_ID_COLOR_REDISH as already mentioned, and "Information" and "Hint" should use KIND_ID_COLOR_BLUISH. |
Other LSP packages depend on it. New signature version named EnhancedLocationPicker.
|
Actually, to be totally honest, I'm very happy that I was able to squeeze in a few days for contributing. LSP is one of my most important dev tools and a big help in my day-to-day work. I think the groundwork for this feature is laid and I wouldn't terribly mind if someone could take from there. You guys know what's planned and how this feature would fit in best, as it is (once the tests pass) as experimental new feature or with the better new ListInputHandler at a later moment. I leave this up to you. |
|
The test failures look really weird. Like not even related. |
|
On my machine I get the same test failure with current main. |
One entry per project file: highest severity in file, path, error/warning counts. ENTER to go to the first diagnostic reported for this file, SHIFT+ENTER to drill down to the by-file diagnostic quick panel. Renamed module, set max details lines to 0.
|
Maybe this is better. |
|
One more thing about the failing tests: When I try to use Pylsp or Pyright with the LSP code itself, I get this exception, no matter whether on this PR or on main: Stack trace only for Pylsp, Pyright startup crashes at the same point. Removing the cache and restarting Sublime doesn't help. With my main project (big multi-folder mixed-languages project) both run fine using this PR. Any idea? |
|
|
... and pass correct session to panel items.
|
After removing the UnitTesting package, it works again. |
For now I have removed the long messages. To have one "details" panel at the bottom, for the selected item, would be much better, I agree. This would also be better than opening the popup in the view behind the panel. Would it work like this with ListInputHandler? At first glance it seems to have a totally different interface. Maybe this could be added at a later time, separate from this PR, by reimplementing the LocationPicker? From this the other uses of the LocationPicker would benefit as well. |
|
I have also minor comments about some things:
|
Yes, no problem, maybe I can give it a try to change from the LocationPicker class to a ListInputHandler after this PR is merged. We would need to see if this would work and be useful for the other overlays too. But for the diagnostics, I could imagine that the ListInputHandler would have another great advantage: the ListInputHandlers work like a stack in the command palette, i.e. if you need multiple inputs, it is possible to open multiple InputHandlers after another, and you can always navigate back with backspace. I guess this could work well for the "Goto Diagnostic in Project" command, which would first show a ListInputHandler with the filenames, and then the one from "Goto Diagnostic" with the list of diagnostics. ("Goto Diagnostic in Project" did only opened the file when I tried it yesterday) Two other comments:
DIAGNOSTIC_KIND_TUPLE = {
DiagnosticSeverity.Error: (sublime.KIND_ID_COLOR_REDISH, "e", "Error"),
DiagnosticSeverity.Warning: (sublime.KIND_ID_COLOR_YELLOWISH, "w", "Warning"),
DiagnosticSeverity.Information: (sublime.KIND_ID_COLOR_BLUISH, "i", "Information"),
DiagnosticSeverity.Hint: (sublime.KIND_ID_COLOR_BLUISH, "h", "Hint"),
} |
Yes to all the suggestions.
I meant to make clear which of the classes are in fact commands and which are support classes that won't work as command themselves. What's the convention for that?
Top.
I changed it to shift+enter, maybe it was that? Alt+enter felt too heavy. For testing I have bound the commands to f8 / shift+f8.
|
# Conflicts: # plugin/core/windows.py
Oh, I wasn't aware of the modifier key. I just tried it again and it works fine, even when selecting an entry with the mouse. Your EnhancedLocationPicker is pretty impressive! But I wonder if the behavior from shift+enter shouldn't be the default (on enter without modifier key)? I don't think I've seen this possibility to use modifier keys in the command palette / list overlays in ST before, so I guess most of the users wouldn't know about this ability. I have a feeling that the ListInputHandler could simplify things a lot here regarding the implementation, and the additional "Base" commands probably wouldn't be needed then. But I must say that it already works pretty well and looks neat. |
Yes. The modifier key approach came from my first take (one entry per diagnostic in the project). It was just easier to try out the simplified version (one entry per file) without going into the location picker code again. Plain
Excellent! Also this stack behaviour you mentioned would be very useful for quickly browsing through the code breakage one would cause with a certain change, in a big project. You said you could maybe give the ListInputHandler a try? |
|
Not entirely on topic of this PR but I've just realized that with #1881 the diagnostics are no longer sorted by location in file. Even more noticeable with this PR since you'd expect sorted order when navigating diagnostics in current file. EDIT: Or this worked by accident in the case I've checked due to diagnostics being sorted by severity?
|
Actually, I don't. Type checkers have to walk their way through the (reverse) dependency tree, therefore chances are good that a code issue that causes subsequent problems in the first place is also reported first. This root issue doesn't have to be an error, it can also show up as a warning. Ordering by severity and/or line number seems to me a bit arbitrary in comparison. That's why I was careful to preserve incoming order. This is the order one also gets when running a compiler on the command line (more or less). But preferences differ, sorting could be offered as a user setting (see my commit message cc2948c). |
|
I'm not sure how much of that applies in LSP. Just the last example shows the case where ordering by location would be more useful since all issues are unrelated. But, you don't have to do anything about that now. We can think about that later since we haven't sorted by location before either, only by severity. EDIT: I guess I could also bring another potential argument for sorting - there could be multiple servers running on the same file and then the results would be even more likely to be unrelated. In that case, navigating by location should be better than jumping around the file. |
|
It absolutely applies to LSP. Not that much in single-file mode, maybe, but when you use it with a project-aware language server and a language with an advanced type system, LSP can really shine. After all, I'm here because awareness of the latter apparently was lost here. I want to bring LSP a bit closer to its potential for languages like that. Sorting as an option I suggested already in my first commit, maybe open a new issue? @jwortmann: Btw. The list input handler rewrite is kind of done, I'm just giving it a spin before committing. |
I just wanted to confirm that we did sort the diagnostics by the location, but it was removed with: |
|
The order in which diagnostics are sent is best understood by the language server that has semantic information about each diagnostic and can do a better job than the editor can. Thus, the language server should have a guarantee that the order in which messages are sent ends up being the same order that users see the diagnostics in. Sorting diagnostics in the editor can be seen as an improvement, but I'd caution against it because many language servers already send diagnostics in a particular order on purpose. Looks like the default LSP for Python isn't smart enough to establish a sensible order, but there are compilers and LSPs that can. As it happens, I've written such logic in the Scala compiler toolchain in the past and that's the current logic used to order Scala diagnostics. I hope I've made the case to not make that a default again. I'm not sure if it's worth having it as an option or just asking the downstream LSP servers such as Python LSP to send messages in a better order. |
Because sorting is out of scope for this PR, I will not try to start a conversation.
Not all language servers can guarantee that. If 3 language servers are running in one file. Which for me is acceptable. I haven't seen Scala diagnostics, so maybe for you having unsorted diagnostics is better. Having an option where like |
Also: - revert LocationPicker to original version - update dependencies.json
|
The last commit adds preview pane and back button. As far as I'm concerned, the feature is finished (I stopped using I suggest to bind "Goto Diagnostic" to Future improvements could be:
Thanks to @jwortmann and @rchl for suggestions and support! |
|
Looks and works nice!
Regarding the diagnostics sorting; I also prefer them sorted by location, so I would be in favor of having a setting for it (in another PR). I would maybe add another option to sort them by severity (and then by location for diagnostics of same severity): // Sort order of diagnostics for each file in the diagnostics panel and the Goto Diagnostics command
"diagnostics_sort_order": "none" | "location" | "severity" |
|
It would be nice to add keybindings but commented out by default. Would help with visibility of that feature. |
|
I've just checked out these changes to try this feature out. The shift+f8 action works well and I can iterate across diagnostics of different buffers, but the f8 action that takes the |
|
The " But I'm not sure if I understand you correctly. There are now the new "LSP: Goto Diagnostic..." / "LSP: Goto Diagnostic in Project..." items in the "Goto" menu. The first one should be enabled when there are diagnostics available for the current file, the second one when there are any diagnostics in the project. Could you please check if this is the case? Do they work? |
|
For me, the keybindings seem to work correctly. Maybe there should be a short status message "No diagnostics for active view" if there are no diagnostics, to make it clear. But I don't know how to achieve this, because to be shown for the keybinding, the status message would have to be set from within the Also, for me the padding between the list items is different dependent on whether the command was triggered by a keybinding / from the menu, or from the command palette. But I assume this must be a ST core bug and nothing we could control. Here with the adaptive theme:
Sometimes the description in the details pane at the bottom is cut off too, if it spans multiple lines, but iirc I've already seen a report in the ST issue tracker for that. And sorry to bother again, but would it look better to remove the codeDescription from the annotation of the list items and only show the server name (source)? Currently the content of the details pane is basically the same of what is shown already in the list items, and one reason for the ListItemHandler was to save some space in the items. But if you/others prefer how it is now, feel free to leave it there. |
| // "context": [ | ||
| // { | ||
| // "key": "setting.lsp_active" | ||
| // } | ||
| // ] |
There was a problem hiding this comment.
I'd say that this one should have no context check because it should still work even if current file doesn't have any session.
There was a problem hiding this comment.
It is supposed to work for all files in project, even for those without a session, and it does.
There was a problem hiding this comment.
It does work indeed but this context check is wrong because, if it would work, it would only allow this command to run when the active file has active session.
The reason it doesn't cause problems is due to this being a WindowCommand so our custom on_query_context handler is not even queried and the check always passes.
There was a problem hiding this comment.
Or more precisely, the setting.lsp_active key is internally matched against the view's settings object but since this is not a TextCommand, it doesn't do what it looks like.
|
The annotation is actually the The padding bug I don't see on the Mac, but sometimes the preview pane partly covers the items. The layout engine seems to be a little buggy. As I already mentioned: As far as I'm concerned, the feature is finished. There is certainly room for improvements (I have mentioned a few points myself), but for now I'm done with it. This is a voluntary contribution, take it or leave it. |
|
It's a great change and thanks for you contribution. We can improve some small things after this is merged if you don't have more resources to work on it. |
| def severity_count(severity: int) -> Callable[[List[Diagnostic]], int]: | ||
| def severity_count(diagnostics: List[Diagnostic]) -> int: | ||
| return len(list(filter(has_severity(severity), diagnostics))) | ||
|
|
||
| return severity_count |
There was a problem hiding this comment.
Can't this be a lambda?
| def severity_count(severity: int) -> Callable[[List[Diagnostic]], int]: | |
| def severity_count(diagnostics: List[Diagnostic]) -> int: | |
| return len(list(filter(has_severity(severity), diagnostics))) | |
| return severity_count | |
| def severity_count(severity: int) -> Callable[[List[Diagnostic]], int]: | |
| return lambda diagnostics: len(list(filter(has_severity(severity), diagnostics))) |
| def is_severity_included(max_severity: int) -> Callable[[Diagnostic], bool]: | ||
| def severity_included(diagnostic: Diagnostic) -> bool: | ||
| return diagnostic_severity(diagnostic) <= max_severity | ||
|
|
||
| return severity_included |
There was a problem hiding this comment.
Lambda?
| def is_severity_included(max_severity: int) -> Callable[[Diagnostic], bool]: | |
| def severity_included(diagnostic: Diagnostic) -> bool: | |
| return diagnostic_severity(diagnostic) <= max_severity | |
| return severity_included | |
| def is_severity_included(max_severity: int) -> Callable[[Diagnostic], bool]: | |
| return lambda diagnostics: diagnostic_severity(diagnostic) <= max_severity |
| def has_severity(severity: int) -> Callable[[Diagnostic], bool]: | ||
| def has_severity(diagnostic: Diagnostic) -> bool: | ||
| return diagnostic_severity(diagnostic) == severity | ||
|
|
||
| return has_severity |
There was a problem hiding this comment.
Maybe turn this into a lambda as well?
| def has_severity(severity: int) -> Callable[[Diagnostic], bool]: | |
| def has_severity(diagnostic: Diagnostic) -> bool: | |
| return diagnostic_severity(diagnostic) == severity | |
| return has_severity | |
| def has_severity(severity: int) -> Callable[[Diagnostic], bool]: | |
| return lambda diagnostic: diagnostic_severity(diagnostic) == severity |
There was a problem hiding this comment.
These I would leave as they are because it's immediately clear from the layout that they are just curried functions, with lambdas this would be a little harder to get.
A proper @curry decorator would be nice, though:
@curry
def is_severity_included(max_severity: ..., diagnostic: ...) -> ...:
return diagnostic_severity(diagnostic) <= max_severityThere is one in the toolz package, but it seems overkill here and I don't know how well it plays with typing.
There was a problem hiding this comment.
I'm personally not accustomed to the use of curried functions. I see a callable as return type and would expect a lambda if it's a single line. I think this is true for a lot of Python (or imperative) developers. But, I guess your argumentation makes sense as well.
There was a problem hiding this comment.
One downside of lambdas is that those can't be type-annotated properly.
There was a problem hiding this comment.
I would recommend functools.partial, but iirc mypy also had problems with that. No idea about pyright.
Currying is really cool, but it's not native to Python and will forever feel alien unless it becomes integrated and more Python developers get to know it. The fact that a calling any function may return another function if it isn't provided with all its requested arguments just doesn't apply to languages without this concept at its core.
Co-authored-by: Raoul Wols <r@primef.actor>
@rchl (#1721 (comment)): This is what I had in mind.
With
lsp_goto_diagnostic/lsp_goto_diagnostic_in_projectyou can launch a quick panel with either the diagnostics for the current file or all diagnostics currently in store. It shows the selected diagnostic in a view behind the panel and opens transient views when necessary. With ENTER you go to the corresponding location. With ALT+ENTER on the project panel you switch to the corresponding per-file panel.It doesn't interfere with
next_result/previous_result.It is already quite usable, especially when bound to shortcuts, but definitely needs some polishing:
Addresses #1696, #1721.