A couple minor fixes for Flymake diagnostics#4435
A couple minor fixes for Flymake diagnostics#4435brownts wants to merge 2 commits intoemacs-lsp:masterfrom
Conversation
| (if-let ((region (flymake-diag-region (current-buffer) | ||
| (1+ start-line) | ||
| character))) | ||
| (1+ character)))) |
There was a problem hiding this comment.
Can you elaborate why +1? 🤔 Thanks!
There was a problem hiding this comment.
flymake-diag-region expects the line and column values to start at 1, not start at 0 like LSP specifies. Thus, both the line and column need incrementing when calling flymake-diag-region. The line number was already incremented but the column was not.
Note that this logic is only executed when the LSP server doesn't support an end position (i.e., the start and end position are the same), so flymake-diag-region is used to compute it.
flymake-diag-region is typically used to turn a normal compiler diagnostic position (1-based) into a region. Since LSP specifies position as 0-based, we need to adjust the line and column numbers before using the function.
Previously, there was a Flycheck specific defcustom, which was used to select a default error level for Flycheck. However, there was nothing for Flymake. Additionally, there were numerous other places in lsp-mode which query the severity attribute of the diagnostic, such as for statistics, modeline, headerline, etc. All of these were being handled inconsistently, either not at all (causing an error when the attribute was not sent by the server), ignoring it when it was missing (causing statistics to be inaccurate) or assuming a default of error, which might have been different than the Flycheck specific configuration, therefore causing an inconsistency in the modeline statistics vs what Flycheck would report. This change creates a common defcustom which is then used anywhere the diagnostic severity is needed, but was not provided by the server. This should create consistent statistics between the modeline and the back-end checkers. Additionally, the mapping between this defcustom and the checker specific values happens in the diagnostic package. Since this defcustom is used outside of the lsp-diagnostic package, it resides in the lsp-mode package and renamed more generally. Additionally, the Flycheck specific defcustom has been obsoleted.
brownts
force-pushed
the
bugfix/flymake-diagnostic
branch
from
71a9a69 to
a686a77
Compare
|
I decided to take a different approach with respect to the default severity level. Instead of adding an additional On a side note, I have also seen |
| (level (cond ((= severity lsp/diagnostic-severity-error) 'error) | ||
| ((= severity lsp/diagnostic-severity-warning) 'warning) | ||
| ((= severity lsp/diagnostic-severity-information) 'info) | ||
| ((= severity lsp/diagnostic-severity-hint) 'info))) |
There was a problem hiding this comment.
Using pcase is probably cleaner.
And the one below, too.
There was a problem hiding this comment.
I tried that at first, but was having trouble with pcase not evaluating the lsp/diagnostic variables. If you have an example of how to transform this, I'd appreciate it.
There was a problem hiding this comment.
I guess this isn't any cleaner. 🤔
(pcase severity
((pred (= lsp/diagnostic-severity-error)) 'error)
((pred (= lsp/diagnostic-severity-warning)) 'warning)
((pred (= lsp/diagnostic-severity-information)) 'info)
((pred (= lsp/diagnostic-severity-hint)) 'info)
(_ 'info))It's okay to leave it as it is.
| (const :tag "Information" info) | ||
| (const :tag "Hint" hint)) | ||
| :group 'lsp-mode | ||
| :package-version '(lsp-mode . "9.0.1")) |
There was a problem hiding this comment.
Should we keep this in lsp-diagnostics.el? 🤔
There was a problem hiding this comment.
Hi @jcs090218, the reason I put it in lsp-mode.el is because there are multiple users, not just lsp-diagnostics.el. lsp-modeline, lsp-headerline, lsp-mode and lsp-diagnostics all need this information, so it seemed more general and likely belongs in lsp-mode.el. It answers the general question of what the user wants to do in the absence of LSP severity information. lsp-diagnostics.el is just one such user of this information.
I placed it in lsp-mode.el near a couple other diagnostic-related user options (i.e., lsp-after-diagnostics-hook and lsp-diagnostics-updated-hook) to be consistent there. It appears that lsp-diagnostics.el is strictly the diagnostics provider (i.e., Flymake/Flycheck) interfacing, but other LSP diagnostics related information is kept in lsp-mode.el.
2 participants
No description provided.