search

haskell / lsp

Haskell library for the Microsoft Language Server Protocol
366 stars 92 forks source link

LSP compliance: handle single MarkedString in Hover #431

Closed thomasjm closed 1 year ago

thomasjm commented 2 years ago

This PR fixes the _contents field of Hover to try parsing a single MarkedString, as per the spec.

To do this I just removed the HoverContents type, which seems kind of unnecessary and inconsistent with the normal style of just using |?.

Getting rid of HoverContents requires minor changes in client code so this is a breaking change. HoverContents also came with a Monoid/Semigroup instance, but I couldn't find any place where it was actually used, either in this repo or haskell-language-server.

An alternative would have been to add another constructor to HoverContents for this case, which would make it a slightly less breaking change, but I was too horrified at the prospect of updating the Semigroup instance with another constructor...

michaelpj commented 2 years ago

To do this I just removed the HoverContents type, which seems kind of unnecessary and inconsistent with the normal style of just using |?.

Please keep the type. We don't have a normal style, it's in fact totally inconsistent and unclear when we use named types and when we use |?, see https://github.com/haskell/lsp/issues/294.

I'm fairly sure that HLS uses the Semigroup instance.