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

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

[Troy Brown]

[-- Attachment #1: Type: text/plain, Size: 1342 bytes --]

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.

[-- Attachment #2: 0001-Eglot-Support-formatting-MarkedString-code-block.patch --]
[-- Type: text/x-patch, Size: 1124 bytes --]

From 4a9412478819c6cb4446d2169901cdb9152a203a Mon Sep 17 00:00:00 2001
From: Troy Brown <brownts@troybrown.dev>
Date: Mon, 3 Jun 2024 22:19:24 -0400
Subject: [PATCH] Eglot: Support formatting MarkedString code-block

* lisp/progmodes/eglot.el (eglot--format-markup): Add support
for MarkedString code-block.

Copyright-paperwork-exempt: yes
---
 lisp/progmodes/eglot.el | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/lisp/progmodes/eglot.el b/lisp/progmodes/eglot.el
index 5ccae5210fe..3a44acdb75b 100644
--- a/lisp/progmodes/eglot.el
+++ b/lisp/progmodes/eglot.el
@@ -1862,7 +1862,8 @@ eglot--format-markup
   (pcase-let ((`(,string ,mode)
                (if (stringp markup) (list markup 'gfm-view-mode)
                  (list (plist-get markup :value)
-                       (pcase (plist-get markup :kind)
+                       (pcase (or (plist-get markup :kind)
+                                  (plist-get markup :language))
                          ("markdown" 'gfm-view-mode)
                          ("plaintext" 'text-mode)
                          (_ major-mode))))))
-- 
2.37.1

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

[Felician Nemeth]

Troy Brown <brownts@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

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

[Troy Brown]

On Tue, Jun 4, 2024 at 1:42 AM Felician Nemeth
<felician.nemeth@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.

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

[Felician Nemeth]

Troy Brown <brownts@troybrown.dev> writes:

> On Tue, Jun 4, 2024 at 1:42 AM Felician Nemeth
> <felician.nemeth@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?

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

[Troy Brown]

[-- Attachment #1: Type: text/plain, Size: 340 bytes --]

On Tue, Jun 4, 2024 at 7:22 AM Felician Nemeth
<felician.nemeth@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.

[-- Attachment #2: 0001-Eglot-Support-formatting-MarkedString-code-block.patch --]
[-- Type: text/x-patch, Size: 1676 bytes --]

From e9c6387eb123aac1fe4741a871be2b68128de41a Mon Sep 17 00:00:00 2001
From: Troy Brown <brownts@troybrown.dev>
Date: Tue, 4 Jun 2024 08:30:53 -0400
Subject: [PATCH] Eglot: Support formatting MarkedString code-block

* lisp/progmodes/eglot.el (eglot--format-markup): Add support
for MarkedString code-block.

Copyright-paperwork-exempt: yes
---
 lisp/progmodes/eglot.el | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/lisp/progmodes/eglot.el b/lisp/progmodes/eglot.el
index 5ccae5210fe..70224d0dcd6 100644
--- a/lisp/progmodes/eglot.el
+++ b/lisp/progmodes/eglot.el
@@ -1861,11 +1861,15 @@ eglot--format-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))))))
+                 (if-let ((language (plist-get markup :language))
+                          (value (plist-get markup :value)))
+                     (list (concat "```" language "\n" value "\n```")
+                           'gfm-view-mode)
+                   (list (plist-get markup :value)
+                         (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)
-- 
2.37.1

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

[Felician Nemeth]

Troy Brown <brownts@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.

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

[Eli Zaretskii]

> Cc: 71353@debbugs.gnu.org,
>  João Távora <joaotavora@gmail.com>
> From: Felician Nemeth <felician.nemeth@gmail.com>
> Date: Tue, 04 Jun 2024 21:36:02 +0200
> 
> Troy Brown <brownts@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?

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

[João Távora]

[-- Attachment #1: Type: text/plain, Size: 722 bytes --]

Can you please resend the patch for inspection?

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

> > Cc: 71353@debbugs.gnu.org,
> >  João Távora <joaotavora@gmail.com>
> > From: Felician Nemeth <felician.nemeth@gmail.com>
> > Date: Tue, 04 Jun 2024 21:36:02 +0200
> >
> > Troy Brown <brownts@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?
>

[-- Attachment #2: Type: text/html, Size: 1394 bytes --]

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

[Eli Zaretskii]

[-- Attachment #1: Type: text/plain, Size: 270 bytes --]

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

Attached.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Eglot-Support-formatting-MarkedString-code-block.patch --]
[-- Type: text/x-patch, Size: 1676 bytes --]

From e9c6387eb123aac1fe4741a871be2b68128de41a Mon Sep 17 00:00:00 2001
From: Troy Brown <brownts@troybrown.dev>
Date: Tue, 4 Jun 2024 08:30:53 -0400
Subject: [PATCH] Eglot: Support formatting MarkedString code-block

* lisp/progmodes/eglot.el (eglot--format-markup): Add support
for MarkedString code-block.

Copyright-paperwork-exempt: yes
---
 lisp/progmodes/eglot.el | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/lisp/progmodes/eglot.el b/lisp/progmodes/eglot.el
index 5ccae5210fe..70224d0dcd6 100644
--- a/lisp/progmodes/eglot.el
+++ b/lisp/progmodes/eglot.el
@@ -1861,11 +1861,15 @@ eglot--format-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))))))
+                 (if-let ((language (plist-get markup :language))
+                          (value (plist-get markup :value)))
+                     (list (concat "```" language "\n" value "\n```")
+                           'gfm-view-mode)
+                   (list (plist-get markup :value)
+                         (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)
-- 
2.37.1

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

[João Távora]

On Sat, Jun 15, 2024 at 1:36 PM Eli Zaretskii <eliz@gnu.org> wrote:
>
> > From: João Távora <joaotavora@gmail.com>
> > Date: Sat, 15 Jun 2024 10:37:48 +0100
> > Cc: Felician Nemeth <felician.nemeth@gmail.com>, Troy Brown <brownts@troybrown.dev>,
> >       71353@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)

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

[Troy Brown]

On Tue, Jun 18, 2024 at 9:39 AM João Távora <joaotavora@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?

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

[Eli Zaretskii]

> From: Troy Brown <brownts@troybrown.dev>
> Date: Sun, 30 Jun 2024 23:28:36 -0400
> Cc: Eli Zaretskii <eliz@gnu.org>, felician.nemeth@gmail.com, 71353@debbugs.gnu.org
> 
> On Tue, Jun 18, 2024 at 9:39 AM João Távora <joaotavora@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.

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

[João Távora]

Eli Zaretskii <eliz@gnu.org> writes:

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

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