GNU bug report logs - #71353
[PATCH] eglot--format-markup doesn't support MarkedString code-blocks

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 71353 in the body.
You can then email your comments to 71353 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Troy Brown <brownts <at> troybrown.dev>
To: bug-gnu-emacs <at> gnu.org
Subject: [PATCH] eglot--format-markup doesn't support MarkedString code-blocks
Date: Mon, 3 Jun 2024 22:51:07 -0400

[Message part 1 (text/plain, inline)]

It appears that eglot--format-markup only supports MarkedString for
string literals and MarkupContent.  This causes markup provided by the
server as a MarkedString code-block to be formatted using the major
mode.  This happens because the MarkedString code-block uses the
"language" field to designate the type of markup.
eglot--format-markup is only looking for the "kind" field found in
MarkupContent.  The following is an example of a hover response from
an LSP server using a MarkedString code-block to provide a description
in "plaintext".

[jsonrpc] e[13:34:28.888] --> textDocument/hover[25]
{"jsonrpc":"2.0","id":25,"method":"textDocument/hover","params":{"textDocument":{"uri":"file:///home/troy/repos/crates/gtkada_24.0.0_80c56171/src/gtkada.gpr"},"position":{"line":70,"character":12}}}
[jsonrpc] e[13:34:29.033] <-- textDocument/hover[25]
{"jsonrpc":"2.0","id":25,"result":{"contents":[{"language":"plaintext","value":"This
package specifies the compilation options used when building an
executable or a library for a project. Most of the options should be
set in one of Compiler, Binder or Linker packages, but there are some
general options that should be defined in this package."}]}}

The following patch will format MarkedString code-blocks for markdown
and plaintext while using the major mode to format any other language.

[0001-Eglot-Support-formatting-MarkedString-code-block.patch (text/x-patch, attachment)]

Message #8 received at 71353 <at> debbugs.gnu.org (full text, mbox):

From: Felician Nemeth <felician.nemeth <at> gmail.com>
To: Troy Brown <brownts <at> troybrown.dev>
Cc: 71353 <at> debbugs.gnu.org
Subject: Re: bug#71353: [PATCH] eglot--format-markup doesn't support
 MarkedString code-blocks
Date: Tue, 04 Jun 2024 07:41:57 +0200

Troy Brown <brownts <at> troybrown.dev> writes:

> The following patch will format MarkedString code-blocks for markdown
> and plaintext while using the major mode to format any other language.

According to the LSP specification, if MarkedString is "{ language:
string; value: string }", then it should be interpreted as this markdown
formatted string:

```${language}
${value}
```

The patch does not implement this behavior.  And as a sidenote,
MarkedString is deprecated.

https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#textDocument_hover

Message #11 received at 71353 <at> debbugs.gnu.org (full text, mbox):

From: Troy Brown <brownts <at> troybrown.dev>
To: Felician Nemeth <felician.nemeth <at> gmail.com>
Cc: 71353 <at> debbugs.gnu.org
Subject: Re: bug#71353: [PATCH] eglot--format-markup doesn't support
 MarkedString code-blocks
Date: Tue, 4 Jun 2024 06:01:05 -0400

On Tue, Jun 4, 2024 at 1:42 AM Felician Nemeth
<felician.nemeth <at> gmail.com> wrote:
>
> According to the LSP specification, if MarkedString is "{ language:
> string; value: string }", then it should be interpreted as this markdown
> formatted string:
>
> ```${language}
> ${value}
> ```
>
> The patch does not implement this behavior.  And as a sidenote,
> MarkedString is deprecated.
>
> https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#textDocument_hover
>

Unless I'm mistaken, I believe it does implement that behavior if the
language is markdown, plaintext or the same as the major mode.  This
seemed to be the intent of the existing implementation which doesn't
support any other options (and falls back to the major mode) and the
reason why I didn't expand on it with this patch.  I agree that the
patch does not support anything outside that behavior.  Additionally,
I don't think that because MarkedString is deprecated is a reason to
implement partial support for it (MarkedString for string literals)
when there are obviously existing LSP servers that utilize it.

Message #14 received at 71353 <at> debbugs.gnu.org (full text, mbox):

From: Felician Nemeth <felician.nemeth <at> gmail.com>
To: Troy Brown <brownts <at> troybrown.dev>
Cc: 71353 <at> debbugs.gnu.org
Subject: Re: bug#71353: [PATCH] eglot--format-markup doesn't support
 MarkedString code-blocks
Date: Tue, 04 Jun 2024 13:21:55 +0200

Troy Brown <brownts <at> troybrown.dev> writes:

> On Tue, Jun 4, 2024 at 1:42 AM Felician Nemeth
> <felician.nemeth <at> gmail.com> wrote:
>>
>> According to the LSP specification, if MarkedString is "{ language:
>> string; value: string }", then it should be interpreted as this markdown
>> formatted string:
>>
>> ```${language}
>> ${value}
>> ```

> Unless I'm mistaken, I believe it does implement that behavior if the
> language is markdown, plaintext or the same as the major mode.  

I don't think "markdown" is a valid markdown language.

For example, if a MarkedString is

    { "language": "c", "value": "printf(42);"}

then the LSP client should interpret it as this markdown-formatted text:

    ```c
    printf(42);
    ```

Whereas I think you argue that this MarkedString:

    { "language": "markdown", "value": "```c\nprintf(42);\n```"}

should be interpreted as this markdown-formatted text:

    ```c
    printf(42);
    ```

I beleive the second example is not in line with the LSP specification.

The patch, however, will format "printf(42);" according to the
major-mode of the current buffer, which is probably c-mode or something
similar.  So this part will usually work.

In any case, why not just turn a MarkedString into a markdown-formatted
text and give it gfm-view-mode?

Message #17 received at 71353 <at> debbugs.gnu.org (full text, mbox):

From: Troy Brown <brownts <at> troybrown.dev>
To: Felician Nemeth <felician.nemeth <at> gmail.com>
Cc: 71353 <at> debbugs.gnu.org
Subject: Re: bug#71353: [PATCH] eglot--format-markup doesn't support
 MarkedString code-blocks
Date: Tue, 4 Jun 2024 08:37:59 -0400

[Message part 1 (text/plain, inline)]

On Tue, Jun 4, 2024 at 7:22 AM Felician Nemeth
<felician.nemeth <at> gmail.com> wrote:
>
> In any case, why not just turn a MarkedString into a markdown-formatted
> text and give it gfm-view-mode?

Thanks for the detailed explanation.  I've created a new patch which
creates a fenced code block and uses gfm-view-mode, as you suggest.

[0001-Eglot-Support-formatting-MarkedString-code-block.patch (text/x-patch, attachment)]

Message #20 received at 71353 <at> debbugs.gnu.org (full text, mbox):

From: Felician Nemeth <felician.nemeth <at> gmail.com>
To: Troy Brown <brownts <at> troybrown.dev>
Cc: 71353 <at> debbugs.gnu.org,
 João Távora <joaotavora <at> gmail.com>
Subject: Re: bug#71353: [PATCH] eglot--format-markup doesn't support
 MarkedString code-blocks
Date: Tue, 04 Jun 2024 21:36:02 +0200

Troy Brown <brownts <at> troybrown.dev> writes:

> I've created a new patch which creates a fenced code block and uses
> gfm-view-mode,

I have not tried it, but it looks good.  Maybe a comment would help to
understand that the new part handles the case when the MarkedString is a
code-block.

At any rate, I CC'd the maintainer of Eglot.

Thanks.

Message #23 received at 71353 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Felician Nemeth <felician.nemeth <at> gmail.com>
Cc: brownts <at> troybrown.dev, 71353 <at> debbugs.gnu.org, joaotavora <at> gmail.com
Subject: Re: bug#71353: [PATCH] eglot--format-markup doesn't support
 MarkedString code-blocks
Date: Sat, 15 Jun 2024 11:12:16 +0300

> Cc: 71353 <at> debbugs.gnu.org,
>  João Távora <joaotavora <at> gmail.com>
> From: Felician Nemeth <felician.nemeth <at> gmail.com>
> Date: Tue, 04 Jun 2024 21:36:02 +0200
> 
> Troy Brown <brownts <at> troybrown.dev> writes:
> 
> > I've created a new patch which creates a fenced code block and uses
> > gfm-view-mode,
> 
> I have not tried it, but it looks good.  Maybe a comment would help to
> understand that the new part handles the case when the MarkedString is a
> code-block.
> 
> At any rate, I CC'd the maintainer of Eglot.

João, any comments or suggestions?

Message #26 received at 71353 <at> debbugs.gnu.org (full text, mbox):

From: João Távora <joaotavora <at> gmail.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 71353 <at> debbugs.gnu.org, Troy Brown <brownts <at> troybrown.dev>,
 Felician Nemeth <felician.nemeth <at> gmail.com>
Subject: Re: bug#71353: [PATCH] eglot--format-markup doesn't support
 MarkedString code-blocks
Date: Sat, 15 Jun 2024 10:37:48 +0100

[Message part 1 (text/plain, inline)]

Can you please resend the patch for inspection?

On Sat, Jun 15, 2024, 09:12 Eli Zaretskii <eliz <at> gnu.org> wrote:

> > Cc: 71353 <at> debbugs.gnu.org,
> >  João Távora <joaotavora <at> gmail.com>
> > From: Felician Nemeth <felician.nemeth <at> gmail.com>
> > Date: Tue, 04 Jun 2024 21:36:02 +0200
> >
> > Troy Brown <brownts <at> troybrown.dev> writes:
> >
> > > I've created a new patch which creates a fenced code block and uses
> > > gfm-view-mode,
> >
> > I have not tried it, but it looks good.  Maybe a comment would help to
> > understand that the new part handles the case when the MarkedString is a
> > code-block.
> >
> > At any rate, I CC'd the maintainer of Eglot.
>
> João, any comments or suggestions?
>

[Message part 2 (text/html, inline)]

Message #29 received at 71353 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: João Távora <joaotavora <at> gmail.com>
Cc: 71353 <at> debbugs.gnu.org, brownts <at> troybrown.dev, felician.nemeth <at> gmail.com
Subject: Re: bug#71353: [PATCH] eglot--format-markup doesn't support
 MarkedString code-blocks
Date: Sat, 15 Jun 2024 15:36:27 +0300

[Message part 1 (text/plain, inline)]

> From: João Távora <joaotavora <at> gmail.com>
> Date: Sat, 15 Jun 2024 10:37:48 +0100
> Cc: Felician Nemeth <felician.nemeth <at> gmail.com>, Troy Brown <brownts <at> troybrown.dev>, 
> 	71353 <at> debbugs.gnu.org
> 
> Can you please resend the patch for inspection?

Attached.

[0001-Eglot-Support-formatting-MarkedString-code-block.patch (text/x-patch, attachment)]

Message #32 received at 71353 <at> debbugs.gnu.org (full text, mbox):

From: João Távora <joaotavora <at> gmail.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 71353 <at> debbugs.gnu.org, brownts <at> troybrown.dev, felician.nemeth <at> gmail.com
Subject: Re: bug#71353: [PATCH] eglot--format-markup doesn't support
 MarkedString code-blocks
Date: Tue, 18 Jun 2024 14:39:20 +0100

On Sat, Jun 15, 2024 at 1:36 PM Eli Zaretskii <eliz <at> gnu.org> wrote:
>
> > From: João Távora <joaotavora <at> gmail.com>
> > Date: Sat, 15 Jun 2024 10:37:48 +0100
> > Cc: Felician Nemeth <felician.nemeth <at> gmail.com>, Troy Brown <brownts <at> troybrown.dev>,
> >       71353 <at> debbugs.gnu.org
> >
> > Can you please resend the patch for inspection?
>
> Attached.

The gist of the patch is correct, but please consider replacing by
this alternative, I don't like if-let inside pcase-let, it's too
complex: the following patch should be "flatter".

diff --git a/lisp/progmodes/eglot.el b/lisp/progmodes/eglot.el
index 6896baf30ce..eabe01a1676 100644
--- a/lisp/progmodes/eglot.el
+++ b/lisp/progmodes/eglot.el
@@ -1863,14 +1863,22 @@ eglot--snippet-expansion-fn
            (apply #'yas-expand-snippet args)))))

 (defun eglot--format-markup (markup)
-  "Format MARKUP according to LSP's spec."
-  (pcase-let ((`(,string ,mode)
-               (if (stringp markup) (list markup 'gfm-view-mode)
-                 (list (plist-get markup :value)
-                       (pcase (plist-get markup :kind)
-                         ("markdown" 'gfm-view-mode)
-                         ("plaintext" 'text-mode)
-                         (_ major-mode))))))
+  "Format MARKUP according to LSP's spec.
+MARKUP is either an LSP MarkedString or MarkupContent object."
+  (let (string mode language)
+    (cond ((stringp markup)
+           (setq string markup
+                 mode 'gfm-view-mode))
+          ((setq language (plist-get markup :language))
+           (setq string (concat "```" language "\n"
+                                (plist-get markup :value) "\n```")
+                 mode 'gfm-view-mode))
+          (t
+           (setq string (plist-get markup :value)
+                 mode (pcase (plist-get markup :kind)
+                        ("markdown" 'gfm-view-mode)
+                        ("plaintext" 'text-mode)
+                        (_ major-mode)))))
     (with-temp-buffer
       (setq-local markdown-fontify-code-blocks-natively t)
       (insert string)

Message #35 received at 71353 <at> debbugs.gnu.org (full text, mbox):

From: Troy Brown <brownts <at> troybrown.dev>
To: João Távora <joaotavora <at> gmail.com>
Cc: 71353 <at> debbugs.gnu.org, Eli Zaretskii <eliz <at> gnu.org>,
 felician.nemeth <at> gmail.com
Subject: Re: bug#71353: [PATCH] eglot--format-markup doesn't support
 MarkedString code-blocks
Date: Sun, 30 Jun 2024 23:28:36 -0400

On Tue, Jun 18, 2024 at 9:39 AM João Távora <joaotavora <at> gmail.com> wrote:
>
> The gist of the patch is correct, but please consider replacing by
> this alternative, I don't like if-let inside pcase-let, it's too
> complex: the following patch should be "flatter".
>

Thanks João, I've tested your patch and verified it addresses my
issue.  Since I'm at my limit for patches without copyright
assignment, can you apply your correction for this?

Message #38 received at 71353 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: joaotavora <at> gmail.com, Troy Brown <brownts <at> troybrown.dev>
Cc: 71353 <at> debbugs.gnu.org, felician.nemeth <at> gmail.com
Subject: Re: bug#71353: [PATCH] eglot--format-markup doesn't support
 MarkedString code-blocks
Date: Sat, 06 Jul 2024 11:41:03 +0300

> From: Troy Brown <brownts <at> troybrown.dev>
> Date: Sun, 30 Jun 2024 23:28:36 -0400
> Cc: Eli Zaretskii <eliz <at> gnu.org>, felician.nemeth <at> gmail.com, 71353 <at> debbugs.gnu.org
> 
> On Tue, Jun 18, 2024 at 9:39 AM João Távora <joaotavora <at> gmail.com> wrote:
> >
> > The gist of the patch is correct, but please consider replacing by
> > this alternative, I don't like if-let inside pcase-let, it's too
> > complex: the following patch should be "flatter".
> >
> 
> Thanks João, I've tested your patch and verified it addresses my
> issue.  Since I'm at my limit for patches without copyright
> assignment, can you apply your correction for this?

João, would you please install your patch?

Thanks.

Message #43 received at 71353-done <at> debbugs.gnu.org (full text, mbox):

From: João Távora <joaotavora <at> gmail.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 71353-done <at> debbugs.gnu.org, Troy Brown <brownts <at> troybrown.dev>,
 felician.nemeth <at> gmail.com
Subject: Re: bug#71353: [PATCH] eglot--format-markup doesn't support
 MarkedString code-blocks
Date: Sat, 06 Jul 2024 10:18:46 +0100

Eli Zaretskii <eliz <at> gnu.org> writes:

> João, would you please install your patch?

Done in the emacs-30 branch.  Closing this bug.

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.