Support MarkupContent[] in a Hover

[DJMcNab]

Fixes #374 and microsoft/language-server-protocol#518.

Supported by microsoft/language-server-protocol#572

Note that StringOrContent is used to ease type-checking, as TS doesn't seem to be able to implicitely convert ls.MarkedString[] | ls.MarkupContent[] to (ls.MarkedString | ls.MarkupContent)[] for Array#map. This does change the semantic meaning of asHoverContent, but it's an internal function anyway, so ¯\_(ツ)_/¯.

[msftclas]

All CLA requirements met.

[dbaeumer]

@DJMcNab can you elaborate why this is necessary. The reason why I am asking is that this is a breaking change for a client and we need to provide capabilities for this.

If the reason for this is the rendering in the VS Code UI (horizontal lines) I think we are better off treating --- special and split the content on the client side into an array there.

[DJMcNab]

Certainly - the reason is for that UI effect. I created this PR mostly because you labelled #374 as 'feature-request', without raising this issue, which then seemed dormant and unactioned. I also encountered this limitation in a language server I am developing, where I had to fall back on the old deprecated API, which seemed wrong. However, seeing @felixfbecker's solution as laid out in microsoft/language-server-protocol#518 (comment) makes me realise that splitting on the --- is a good idea.

However, the usual visual style of the --- is ugly, so perhaps it would be good to clarify that client implementations should use minimal styling for a --- in markdown for a hover.

[dbaeumer]

@DJMcNab I think the styling of --- is very specific to VS Code and how it combines hovers from different providers. In VS Code for a language X there can be two hover providers which are separated using a hline. The API allows to separate section with a different styled hline by providing an array. IMO this shouldn't leak into the LSP API. So we should simple here https://github.com/Microsoft/vscode-languageserver-node/blob/master/client/src/protocolConverter.ts#L257 convert the hover content into an array based on ---.

[DJMcNab]

So, just to clarify, I should close microsoft/language-server-protocol#572 and instead make the change in https://github.com/Microsoft/vscode-languageserver-node/blob/master/client/src/protocolConverter.ts#L257.

What would be the best approach to this - should I force push an update? Is it worth the slight jankiness of splitting based on the raw string ---. Alternatively, the actual vscode hover markdown support replace Horizontal Break with the thinner line style.

[DJMcNab]

There, I have added the suggested changes.

Note that Sourcegraph uses css rules to make the markdown content with horizontal seperators look the same as multiple array items: see sourcegraph/codeintellify@2ab2bf6.

@felixfbecker, do you think we should use this approach in vscode, although (I think) that would require a PR against Microsoft/vscode.

[felixfbecker]

Is this correct? What if --- appears in a code snippet, or in the middle of a sentence?

[dbaeumer]

I am currently fixing it :-) We need to consider *** and ___ as well and ignore html comments

[dbaeumer]

@DJMcNab I reverted the merge. The think is more complicated than looking for ---

• things we need to do: consider *** and ___ as well.
• handle HTML comments correctly
• test showed that the hline needs to start at the beginning of a line or has less than three spaces in front. But I am not sure this is standard.

Are you willing to continue on this?

[dbaeumer]

Actually this is getting too complicated. We would need to handle hline as well and ignore --- in inline html. Doing this right would almost mean to convert the whole markdown. Will think about another solution.

[DJMcNab]

OK! Well I'm glad we're thinking about a solution! Don't worry about it.

[dbaeumer]

Fixed via: microsoft/vscode#65094

The following code:

connection.onHover((textPosition): Hover => {
	// connection.tracer.log('A trace message from the server', 'Verbose content');
	return {
		contents: {
			kind: MarkupKind.Markdown,
			value: [
				'```typescript',
				'function validate(document: TextDocument): Diagnostic[]',
				'```',
				'___',
				'Some doc',
				'',
				'_@param_ `document` '
			].join('\n')
		}
	};
});

produces the following hover:

[felixfbecker]

Nice, that's the solution we chose in Sourcegraph too 👍

[DJMcNab]

Awesome work! Thanks.

Should we have a note about this in the protocol document - I suppose microsoft/language-server-protocol#518 should track that?

Also, does tsserver currently use this or does it use the deprecated MarkedString[] approach.

[dbaeumer]

TSServer does use the deprecated MarkedString[] appraoch.

I also updated microsoft/language-server-protocol#518

@DJMcNab what do you suggest to add to the protocol document ?

[DJMcNab]

Something like:

It is recommended that implementations display markdown seperators (such as denoted by --- or <hr/>) as a minimal horizontal line, as can be seen in vscode.

Maybe in slightly better English 😃.

[dbaeumer]

I think this is not a good idea since the LSP purposely stayed away from defining how something should be rendered. If there is an editor that renders <hr/> as a bold thick line and does this consistently I have no problems with that from a LSP perspective.

[DJMcNab]

Yeah, that's fine. It might be worth noting that --- (or <hr />) is the standard separator (such as between definitions and documentation or conflicting definitions), to clarify the situation for servers. Otherwise, it can just be left.