Apply some tweaks to content of hover popups

[jwortmann]

Here is a suggestion with some enhancement for pyright's hover popups:

1. Fix horizontal rule after fenced code block accidentally being rendered as setext heading (this seems to be a bug in mdpopups or in one of its dependencies)
2. Improve rendering for some common reStructuredText field names in docstrings (ideally pyright should parse the docstrings itself and should have this built-in...)

Before:

After:

[jfcherng]

Awesome. I would like it to support Sphinx style too! (type and rtype)

def foo(arg1: int, arg2: str) -> bool:
    """
    { function_description }

    :param      arg1:  The argument 1
    :type       arg1:  int
    :param      arg2:  The argument 2
    :type       arg2:  str

    :returns:   { description_of_the_return_value }
    :rtype:     bool
    """

[rchl]

Let's first try to suggest this to pyright or even help implementing in pyright. Including a parser for whole new syntax in LSP-pyright seems a bit excessive.

[jwortmann]

Let's first try to suggest this to pyright or even help implementing in pyright. Including a parser for whole new syntax in LSP-pyright seems a bit excessive.

According to microsoft/pylance-release#1113 (comment) it is intended to not apply any formatting to the Sphinx-style (reST) field lists in hover popups from the Pylance extension for VSCode, and render them as raw text instead. I also found microsoft/pylance-release#2251 and a few more similar requests, but it appears that there is no real action taken or deemed to be necessary about this.

[rchl]

They are not strictly against improving it:

Feel free to file a new issue if you have an idea of what you'd prefer instead.

They base the feature requests on interest. Good start would be creating a specific issue for formatting those params better (not saying you should but you could :)).

[jwortmann]

Good start would be creating a specific issue for formatting those params better

There are already plenty of issue reports about this. If it gets created in the microsoft/pyright repository, it will be transfered to microsoft/pylance-release because it isn't a core type checking feature and is related instead to language server features (microsoft/pylance-release#2294 (comment)).

After that, it will be converted to a discussion, so that it can easily be put on ice / ignored.
Besides the two links I've already referenced in the previous comment, there are some more duplicates about this:

• Improve the readability of function definition microsoft/pylance-release#3185
• Show parameters description using python doctype microsoft/pylance-release#1130
• ...

I don't really see a reason why we should create another one of it.

[rchl]

Yeah ok, seems like the first one is exactly what we needed.

[rchl]

Thumbs up on microsoft/pylance-release#3186 wouldn't hurt. This might be how they gauge interest.

[rchl]

It looks like they are open to improving this (see the link above) but would want to see a more formal enhancement suggestion first. I suppose it would be a short comment describing the problem, motivation for changing and potentially suggested solution. I could write something but you are much better at english then me and you already did a lot of work on that. :)

[rchl]

In the mean time I'm fine with merging this but ideally it would wait for the LSP fix to be released first.

[jwortmann]

Well, I think the problem and motivation is pretty clear already from the screenshot in the opening comment of that discussion, and from all the previous enhancement requests.
I assume they demand some kind of draft how the result should look and how exactly you would implement it. Just to avoid that you spend time on something which perhaps would not be merged in the end.
For example the simple workaround with the regex-substitution I use here in this PR, and resulting in the Params: prefixes on each of the parameter lines, wouldn't be very satisfying I guess. Instead, you would need to collect all of the parameters first and then create an unordered list in Markdown with the parameters, for example, maybe also add a dash between parameter name and parameter description (dependent whether description exists or not), etc.
But since pyright is implemented in TypeScript, and I have absolutely zero experience with TypeScript (besides from looking at the LSP specs), I guess I wouldn't want to spend the time to figure out an implementation and the details. If you want to invest the time for an implementation, I wouldn't complain of course, but for me personally the simple approach from this PR is sufficient for now, and it was quick and easy to write just a handful lines of Python code.

but you are much better at english then me

disagree 😄

[rchl]

Let's first try to suggest this to pyright or even help implementing in pyright. Including a parser for whole new syntax in LSP-pyright seems a bit excessive.

[rchl]

There is something wrong with the fixup. Note extra newlines and :returns not being formatted.

Before:

After:

[jwortmann]

:returns is not being converted, because it is not a valid reStructuredText field name (it should be :returns: instead).

I'll try to find out why the increased line spacing / additional newlines in the completion docs happen, but from the implementation of this PR it's not obvious to me, because the \n is just retained in the regex substitutions and there shouldn't be more newlines added.

[rchl]

Hmm, strange. Before and after modification:

```python
on_workspace_configuration(self: Self@AbstractPlugin, params: Dict[Unknown, Unknown], configuration: Any) -> Any
```
---
Override to augment configuration returned for the workspace/configuration request.

:param      params:         A ConfigurationItem for which configuration is requested.  
:param      configuration:  The pre-resolved configuration for given params using the settings object or None.

:returns The resolved configuration for given params.

```python
on_workspace_configuration(self: Self@AbstractPlugin, params: Dict[Unknown, Unknown], configuration: Any) -> Any
```

---
Override to augment configuration returned for the workspace/configuration request.

__Param:__ `params`         A ConfigurationItem for which configuration is requested.  
__Param:__ `configuration`  The pre-resolved configuration for given params using the settings object or None.

:returns The resolved configuration for given params.

[rchl]

It's this replace that causes it for some reason: content = re.sub("```\n---", "```\n\n---", content)

HTML before:

<h2><div class="highlight"><pre><span style="color: #d8dee9;">on_workspace_configuration</span><span style="color: #ffffff;">(</span><span style="color: #f9ae58;">self</span><span style="color: #a6acb9;">:</span><span style="color: #d8dee9;"> </span><span style="color: #d8dee9;">Self</span><span style="color: #f97b58;">@</span><span style="color: #d8dee9;">AbstractPlugin</span><span style="color: #a6acb9;">,</span><span style="color: #d8dee9;"> </span><span style="color: #f9ae58;">params</span><span style="color: #a6acb9;">:</span><span style="color: #d8dee9;"> </span><span style="color: #d8dee9;">Dict</span><span style="color: #ffffff;">[</span><span style="color: #d8dee9;">Unknown</span><span style="color: #a6acb9;">,</span><span style="color: #d8dee9;"> </span><span style="color: #d8dee9;">Unknown</span><span style="color: #ffffff;">]</span><span style="color: #a6acb9;">,</span><span style="color: #d8dee9;"> </span><span style="color: #f9ae58;">configuration</span><span style="color: #a6acb9;">:</span><span style="color: #d8dee9;"> </span><span style="color: #d8dee9;">Any</span><span style="color: #ffffff;">)</span><span style="color: #d8dee9;"> </span><span style="color: #a6acb9;">-&gt;</span><span style="color: #d8dee9;"> </span><span style="color: #d8dee9;">Any</span><br></pre></div></h2>
<p>Override to augment configuration returned for the workspace/configuration request.</p>
<p>:param      params:         A ConfigurationItem for which configuration is requested.</p>
<p>:param      configuration:  The pre-resolved configuration for given params using the settings object or None.</p>
<p>:returns: The resolved configuration for given params.</p>

HTML after:

<div class="highlight"><pre><span style="color: #d8dee9;">on_workspace_configuration</span><span style="color: #ffffff;">(</span><span style="color: #f9ae58;">self</span><span style="color: #a6acb9;">:</span><span style="color: #d8dee9;"> </span><span style="color: #d8dee9;">Self</span><span style="color: #f97b58;">@</span><span style="color: #d8dee9;">AbstractPlugin</span><span style="color: #a6acb9;">,</span><span style="color: #d8dee9;"> </span><span style="color: #f9ae58;">params</span><span style="color: #a6acb9;">:</span><span style="color: #d8dee9;"> </span><span style="color: #d8dee9;">Dict</span><span style="color: #ffffff;">[</span><span style="color: #d8dee9;">Unknown</span><span style="color: #a6acb9;">,</span><span style="color: #d8dee9;"> </span><span style="color: #d8dee9;">Unknown</span><span style="color: #ffffff;">]</span><span style="color: #a6acb9;">,</span><span style="color: #d8dee9;"> </span><span style="color: #f9ae58;">configuration</span><span style="color: #a6acb9;">:</span><span style="color: #d8dee9;"> </span><span style="color: #d8dee9;">Any</span><span style="color: #ffffff;">)</span><span style="color: #d8dee9;"> </span><span style="color: #a6acb9;">-&gt;</span><span style="color: #d8dee9;"> </span><span style="color: #d8dee9;">Any</span><br></pre></div>

<hr />
<p>Override to augment configuration returned for the workspace/configuration request.</p>
<p>Param: <code class="highlight"><span style="color: #d8dee9;">params</span></code>         A ConfigurationItem for which configuration is requested.</p>
<p>Param: <code class="highlight"><span style="color: #d8dee9;">configuration</span></code>  The pre-resolved configuration for given params using the settings object or None.</p>
<p>Returns: The resolved configuration for given params.</p>

[jwortmann]

This seems to be a bug in the content handling in LSP. For the completion docs there is another <p> ... </p> added around the entire content, and since nested paragraphs are not allowed in HTML (I think), the following nested <p> for each items are ignored.

The bug is probably somewhere at https://github.com/sublimelsp/LSP/blob/2e2b3d45ada1d1e26c6f8aa745d4a80ef436412d/plugin/core/views.py#L716-L717
Edit: or maybe not, actually it's not around the entire content. Not sure where this <p> ... </p> comes from...
Also my reasoning about ignored nested paragraphs would have the opposite effect, i.e. if it were the cause, it should increase the spacing in hover popups but not in the completion docs.

[rchl]

True that it's not allowed to nest any block elements within p.
Though I've changed it to div and it still reproduces. But to be fair p's default style actually has some margin typically so new behavior actually looks more correct (but not really better). I think we might want to reset the margin's for p in this case but also I have to investigate more as it could easy result in worse rendering in other cases...

[jwortmann]

Actually I think the increased spacing is caused by additional <br /> after each line in the completion docs.

Relevant code which could cause this might be at
https://github.com/sublimelsp/LSP/blob/2e2b3d45ada1d1e26c6f8aa745d4a80ef436412d/plugin/hover.py#L273
and
https://github.com/sublimelsp/LSP/blob/2e2b3d45ada1d1e26c6f8aa745d4a80ef436412d/plugin/completion.py#L87

[rchl]

But there is no extra br's. The output is in #207 (comment)

[jwortmann]

For me, there are addional brs....

This is with jfcherng's code example from above:

[rchl]

True, I've been logging before mdpopups.show_popup call.

[rchl]

For reference, here is final output of mdpopups for the case I've been pasting before (skipped CSS as it's not relevant).

BEFORE:

<div class="mdpopups">
    <div class="lsp_popup">
        <h2>
            <div class="highlight">
                <pre><span style="color: #d8dee9;">on_workspace_configuration</span><span style="color: #ffffff;">(</span><span style="color: #f9ae58;">self</span><span style="color: #a6acb9;">:</span><span style="color: #d8dee9;"> </span><span style="color: #d8dee9;">Self</span><span style="color: #f97b58;">@</span><span style="color: #d8dee9;">AbstractPlugin</span><span style="color: #a6acb9;">,</span><span style="color: #d8dee9;"> </span><span style="color: #f9ae58;">params</span><span style="color: #a6acb9;">:</span><span style="color: #d8dee9;"> </span><span style="color: #d8dee9;">Dict</span><span style="color: #ffffff;">[</span><span style="color: #d8dee9;">Unknown</span><span style="color: #a6acb9;">,</span><span style="color: #d8dee9;"> </span><span style="color: #d8dee9;">Unknown</span><span style="color: #ffffff;">]</span><span style="color: #a6acb9;">,</span><span style="color: #d8dee9;"> </span><span style="color: #f9ae58;">configuration</span><span style="color: #a6acb9;">:</span><span style="color: #d8dee9;"> </span><span style="color: #d8dee9;">Any</span><span style="color: #ffffff;">)</span><span style="color: #d8dee9;"> </span><span style="color: #a6acb9;">-&gt;</span><span style="color: #d8dee9;"> </span><span style="color: #d8dee9;">Any</span><br></pre>
            </div>
        </h2>

        <p>Override to augment configuration returned for the workspace/configuration request.</p>

        <p>Param: <code class="highlight"><span style="color: #d8dee9;">params</span></code> A ConfigurationItem for which configuration is requested.</p>

        <p>Param: <code class="highlight"><span style="color: #d8dee9;">configuration</span></code> The pre-resolved configuration for given params using the settings object or None.</p>

        <p>Returns: The resolved configuration for given params.</p>

        <div class="lsp_popup--spacer"></div>
    </div>
</div>

AFTER:

<div class="mdpopups">
    <div class="lsp_popup">
        <div class="highlight">
            <pre><span style="color: #d8dee9;">on_workspace_configuration</span><span style="color: #ffffff;">(</span><span style="color: #f9ae58;">self</span><span style="color: #a6acb9;">:</span><span style="color: #d8dee9;"> </span><span style="color: #d8dee9;">Self</span><span style="color: #f97b58;">@</span><span style="color: #d8dee9;">AbstractPlugin</span><span style="color: #a6acb9;">,</span><span style="color: #d8dee9;"> </span><span style="color: #f9ae58;">params</span><span style="color: #a6acb9;">:</span><span style="color: #d8dee9;"> </span><span style="color: #d8dee9;">Dict</span><span style="color: #ffffff;">[</span><span style="color: #d8dee9;">Unknown</span><span style="color: #a6acb9;">,</span><span style="color: #d8dee9;"> </span><span style="color: #d8dee9;">Unknown</span><span style="color: #ffffff;">]</span><span style="color: #a6acb9;">,</span><span style="color: #d8dee9;"> </span><span style="color: #f9ae58;">configuration</span><span style="color: #a6acb9;">:</span><span style="color: #d8dee9;"> </span><span style="color: #d8dee9;">Any</span><span style="color: #ffffff;">)</span><span style="color: #d8dee9;"> </span><span style="color: #a6acb9;">-&gt;</span><span style="color: #d8dee9;"> </span><span style="color: #d8dee9;">Any</span><br></pre>
        </div>

        <p>
            <hr /><br />
        <p>Override to augment configuration returned for the workspace/configuration request.</p><br />
        <p>Param: <code class="highlight"><span style="color: #d8dee9;">params</span></code> A ConfigurationItem for which configuration is requested.</p><br />
        <p>Param: <code class="highlight"><span style="color: #d8dee9;">configuration</span></code> The pre-resolved configuration for given params using the settings object or None.</p><br />
        <p>Returns: The resolved configuration for given params.</p>
        <div class="lsp_popup--spacer"></div>
        </p>
    </div>
</div>

[rchl]

We're running mdopups.show_popup on the content from #207 (comment) with md=True set so it processes the content as markdown even though it's already converted from markdown to html.

And somehow it only messes up the code with the version after those changes (with <hr /> present and all).

Can't tell without investigating more whether it's safe to switch to md=False here or it would then result in some markdown content not being parsed.

More minimal example:

mdpopups.show_popup(view, '''
<hr />
<p>Override to augment configuration returned for the workspace/configuration request.</p>
<p>Param 2</p>
<p>Param 1</p>''', md=True)

[rchl]

Created sublimelsp/LSP#2146 with a (hopeful) fix.

[jwortmann]

Seems to look fine now for the completion docs, after your fix was merged.

[rchl]

I will likely release new LSP version tomorrow (if nothing comes up).

[jwortmann]

I wonder can it be merged or should I close this?
A new LSP version with the fix was already released, and I would guess that a potential enhancement in Pyright itself will take a while until someone makes an implementation and it gets accepted. Also that still wouldn't fix the "horizontal rule after fenced code block" issue, because that one is on mdpopups' side.

[rchl]

I wonder can it be merged or should I close this?

I'm fine with merging.

Also that still wouldn't fix the "horizontal rule after fenced code block" issue, because that one is on mdpopups' side.

That one was fixed in LSP and is also released already.

[jwortmann]

That one was fixed in LSP and is also released already.

I still see it happening with latest LSP and LSP-pyright. I think you had something different in mind.

[rchl]

Right. This PR fixes that issue.