Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix wrong-type-argument when MarkedString is a plist #72

Merged
merged 1 commit into from
Aug 17, 2018
Merged

Fix wrong-type-argument when MarkedString is a plist #72

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 1 addition & 2 deletions eglot.el
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1353,8 +1353,7 @@ is not active."
(let ((heading (and range (pcase-let ((`(,beg . ,end) (eglot--range-region range)))
(concat (buffer-substring beg end) ": "))))
(body (mapconcat #'eglot--format-markup
(append (cond ((vectorp contents) contents)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't solve the issue completely as contents can be strings, vectors or plist. The plist itself can be of two types: MarkedString with language and MarkupContent. Also, according to the LSP docs, MarkedString is deprecated for hover response.

I think a correct solution would be something like

(defun eglot--parse-hover-contents (contents)
  "Doc."
  (cond
   ;; string
   ((stringp contents) (eglot--format-markup contents))
   ;; MarkedString[]
   ((vectorp contents) (mapconcat #'eglot--parse-hover-contents contents "\n"))
   ((listp contents)
    (if (plist-get contents :language)
        ;; MarkedString
        (eglot--format-markup contents)
      ;; MarkupContent
      (eglot--format-markup (plist-get contents :value))))))

(let ((heading ...)
       (body (eglot--parse-hover-contents contents))

This is untested, but in concept it should work.

Copy link
Collaborator Author

@mkcms mkcms Aug 13, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, I think it will work just because MarkedString (when it's a plist) and MarkupContent both have a :value key and eglot--format-buffer will silently handle an error in case it's argument is a MarkupContent and has no :language key. We should also handle MarkupContent's :kind though, but that's another issue.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you can also merge it with eglot--format-markup to simplify the implementation and support all kinds of documentation easily.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with @mkcms. eglot--format-markup could see a clarification of the accepted datatypes in the docstring though, and indeed is the place where full support for MarkupContent should be implemented.

((stringp contents) (list contents)))) "\n")))
(if (vectorp contents) contents (list contents)) "\n")))
(when (or heading (cl-plusp (length body))) (concat heading body))))

(defun eglot--sig-info (sigs active-sig active-param)
Expand Down