feat: append LSP diagnostic codes to flymake messages#5036
Conversation
|
Can you do this to |
|
Hi @jcs090218, thanks for the quick reply already. I checked flycheck. The good news is that we don't need to change anything in the flycheck branch. The code is already correct and exactly displays the error code as [CODE]. You can see that code? was already deconstructed and passed to (-map (-lambda ((&Diagnostic :message :severity? :tags? :code? :source?
:range (&Range :start (start &as &Position
:line start-line
:character start-character)
:end (end &as &Position
:line end-line
:character end-character))))
(flycheck-error-new
:buffer (current-buffer)
:checker checker
:filename buffer-file-name
:message message
:level (lsp-diagnostics--flycheck-calculate-level severity? tags?)
:id code?
:group source?
:line (lsp-translate-line (1+ start-line))
:column (1+ (lsp-translate-column start-character))
:end-line (lsp-translate-line (1+ end-line))
:end-column (unless (lsp--position-equal start end)
(1+ (lsp-translate-column end-character))))))What was missing was precisely the flymake branch where the code was not displayed. If this PR makes it to upstream, then both branches display the same message and error code. |
|
Hi @jcs090218 and maintainers, I looked into the three failing CI checks (the While I have your attention, would you be open to a follow-up that makes this more configurable per client? Two directions where I'd appreciate your feedback:
My motivation is that for some servers the codes are essential (Pyright, tsc), while for others they're just visual noise on every line. Happy to prepare a demo PR for whichever direction you prefer, either extending this one, or as a separate PR once this lands. Let me know what you think is the best. |
Yeah, there are breaking changes on the Emacs snapshot. No need to worry about those job failures (at least for now). :)
I prefer the Composable formatter approach because it's more flexible and gives users greater control. Thanks for bringing this up! :D |
d468183 to
723a064
Compare
Add `lsp-diagnostics-flymake-message-formatter', a defcustom that decides how an LSP `Diagnostic' plist is rendered into the single string flymake stores per diagnostic. The default `lsp-diagnostics-flymake-default-message-formatter' reproduces the existing behaviour: append `[code]' when the diagnostic carries one, and return the bare message otherwise. Unlike flycheck — which holds the message, id, severity, and checker on separate slots of `flycheck-error' and lets each consumer (the error list, `sideline-flycheck', etc.) compose them at display time — flymake exposes only `flymake-diagnostic-text'. Any presentation choice therefore has to be made before the diagnostic is constructed. The new hook provides that single point of customisation, letting users suppress the code, prepend the source, add a severity prefix, or reformat the message in any other way without overriding `lsp-diagnostics--flymake-update-diagnostics'.
723a064 to
0057ce3
Compare
|
Dear Jen-Chieh (@jcs090218), Profiting from some public holiday in Germany, I went back to this PR. I'll split my reply into two GitHub comments to keep concerns separate. The first message, provided directly here below, closes the loop on the original PR; the other comment addresses the composable-formatter follow-up you asked for.
Thanks for confirming the CI snapshot failures are unrelated. About the rendering inconsistency you spotted: when I said flycheck I found out that the problem was in the other package To fix it, I have opened a small companion PR to
With that PR and this one, the two frontends are aligned: any user can The first commit on this branch (19d5f31 — feat: append LSP By the way — having now spent some time inside |
I went ahead and implemented that. It's on the same branch as a
I am happy to split this into its own PR if you'd rather review and land What was added in the second commitA defcustom ;; Suppress the bracketed code:
(setq lsp-diagnostics-flymake-message-formatter
(lambda (diag) (lsp-get diag :message)))
;; Prepend the source (e.g. "basedpyright: foo is unused [reportXxx]"):
(setq lsp-diagnostics-flymake-message-formatter
(lambda (diag)
(let ((src (lsp-get diag :source?))
(msg (lsp-get diag :message))
(code (lsp-get diag :code?)))
(concat (and src (format "%s: " src))
msg
(and code (format " [%s]" code))))))Why a hook makes sense on flymake but not on flycheckThe two diagnostics frontends have very different architectures, and flycheck stores structured fields on
So a flycheck user wanting a different rendering already has a knob, flymake, by contrast, exposes a single That means flymake is the only place where lsp-mode is forced to Test plan (verified on my local fork)
Verified end to end using basedpyright on a Python buffer. |
I'm sorry I didn't brought up this in the first place! 😓
I'm glad you like it! :D |
jcs090218
left a comment
There was a problem hiding this comment.
Sounds good to me.
Can you update the changelog as well? Thank you! :D
| reformat the message in any other way." | ||
| :type 'function | ||
| :group 'lsp-diagnostics | ||
| :package-version '(lsp-mode . "9.1.0")) |
There was a problem hiding this comment.
The version number is now 10.0.1. :)
One entry per commit: - the composable `lsp-diagnostics-flymake-message-formatter' defcustom, - the default-on append of the LSP diagnostic code to flymake messages.
Awesome. I just updated the CHANGELOG.org PS: I realized that I never updated the CHANGELOG.org for the other 4 PRs that already landed upstream. There is also one more PR #5057 of mine under review. If you'd like, I could could post changelog suggestions in the comments of the respective PR pages so that you can push those suggestions upstream, or simply as a single block for easy copy&paste for you. These other PRs were about under-the-hood bug fixes. But I'm not sure if you'd want them in the CHANGELOG visible to the users... |
No worries about those PRs. No, every PR needs to update the CHANGELOG. Normally, we don't add a changelog entry for bug fixes unless it's critical and worth mentioning. 🤔 |
|
Merged! Thank you! |
|
Thanks for the review work and always the prompt replies! |
Currently,
lsp-diagnostics--flymake-update-diagnosticsextracts:messageand:severity?from each LSP diagnostic but silently drops:code?. As a result, flymake displays messages like:without the diagnostic rule name that the language server also provides.
This patch binds
:code?and appends it to the message text when present:This is a small but valuable change. The diagnostic code is the primary handle users need to:
# pyright: ignore[reportPossiblyUnbound])Without the code, users must guess the rule name. The fix is a two-line change: bind
:code?in the existing-let*destructuring and conditionally append[code]to the message.The code field is optional in the LSP spec, so the format falls back gracefully to the plain message when absent.