Subject: bug#77439: [PATCH] Eglot: introduce eglot-show-diagnostics-source

From: João Távora <[email-masked]>
Date: Thu, 03 Apr 2025 09:51:36 +0100

Nicolás Ojeda Bär <[email-masked]> writes:

> Dear João,
>
> Thanks for the quick reply. I'm including here a revised version of the patch.
>
> On Tue, Apr 1, 2025 at 11:46 PM João Távora <[email-masked]> wrote:
>>
>> Thanks.   The patch looks good and ticks all the boxes.  Well almost,
>> there  are some minor details I didn't tell you about and are easy to miss.
>>
>> * two spaces between sentences in documentation.
>
> Fixed, thanks.
>
>> * the first line of the docstring should be a complete sentence and
>> fit in a line when the docstring is formatted with M-q
>
> Fixed, thanks.
>
>> * perhaps some other micro things I missed
>> * the code used concat to handle the nilness of source/code, this one
>> is more explicit.  i can live with that though, maybe it's better like this.
>
> OK, I left this part of the code as it is.
>
>> The most important and boring thing will be to figure out if you need
>> a copyright assignment for this contribution.  Maybe not, if it counts
>> as a "trivial" patch, but you should probably get one started
>> regardless.  Eli, please advise.
>
> Actually, I believe I have a copyright assignment on file from long time ago.

Thanks for this patch, as I said, it looks great, exceptionally tidy,
and done in record time.

But I'm very sorry if I'm going to disappoint you.  I've had a change of
heart.  I was thinking about this, and I don't think this customization
belongs in Eglot.  Instead, it belongs to Flymake.

Applying your patch would violate the principle that Eglot as a medium
between LSP and Emacs facilities should not withhold information from
Emacs.

Also, Flycheck store those things itself from the beginning, and it's a
good idea we should copy.  And I've been meaning to do it anyway for a
long time, now is a good opportunity.

So we have to change Flymake somehow so that it becomes aware that
diagnostic messages may have three parts:

- a source
- a code
- a message

Can they have more than three?  Can they have just 2?  Are they to be
named always "source/code/message" like LSP/Flycheck wants us to?  This
is where the under/overengineering antennae should stick up.

If we accept that it's at most 3, and they have those names, the patch
to present to flymake.el is somewhat straightforward:

* change the public flymake-make-diagnostic interface to explicitly
  allow backends to pass in those things.  Since it's sadly BOA
  function, I'd say the best is to overload its TEXT argument, renaming
  it INFO to allow it to be more a list of (SOURCE CODE MESSAGE) instead
  of just a string.

* change the code around to account for the above.  I recommend changing
  the private flymake--diag struct creating flymake--diag-accessor
  macros flymake-diagnostic-source and flymake-diagnostic-code.

* Introduce a flymake-diagnostic-display-info customization option where
  the user can say whether what Flymake gives to its associated
  displaying outlets.  Here I suppose your personal preference would be
  to set this variable to just `(:message).

* The same variable could be repurposed for controlling the columns of
  the flymake-show-diagnostics-buffer tabular display (which would also
  have to tweak.)

* Finally, change Eglot to pass those things in to Flymake.

* Bump the Flymake version, change Eglot's Package-Requires to rely on
  the bumped version.

* Update the flymake.texi manual.

* Run Flymake unit tests.

* Ideally add a new test or two (not neded, but nice)

* Change Emacs's etc/NEWS to reflect the Flymake change.

* Optionally change Emacs's etc/EGLOT-NEWS to call attention to Eglot
  making use of this new Flymake capability.

Of course, if you think this solution is underengineered and we should
have more flexibility, other solutions are possible via a protocol
different from flymake-make-diagnostic.

João