dotnet / roslyn

The Roslyn .NET compiler provides C# and Visual Basic languages with rich code analysis APIs.
https://docs.microsoft.com/dotnet/csharp/roslyn-sdk/
MIT License
19.04k stars 4.03k forks source link

Fix LSP tooltips that are generated as markdown from doc comments #53358

Open Youssef1313 opened 3 years ago

Youssef1313 commented 3 years ago

Few lines above this, there is:

                if (!isInCodeBlock)
                    text = text.Replace(" ", " ");

This doesn't feel correct. See the following test:

https://github.com/dibarbet/roslyn/blob/0ef88b0a686892546b57e721ba65738872bf928d/src/Features/LanguageServer/ProtocolUnitTests/Hover/HoverTests.cs#L195

@sharwell Do you agree here? Do you want me to fix LSP in this PR or make that a separate PR? (I prefer LSP work to be separate - but no objection doing it in this PR)

cc: @dibarbet

_Originally posted by @Youssef1313 in https://github.com/dotnet/roslyn/pull/53140#discussion_r630828905_

Youssef1313 commented 3 years ago
dibarbet commented 3 years ago

Markdown ignores normal leading spaces, so the following written in markdown

Exceptions:
    XYZ

renders as the below Exceptions: XYZ

  ensures the whitespace formatting is kept, for example the following markdown

Exceptions:
    XYZ

renders as this

Exceptions:     XYZ

If you have a better method to ensure the whitespace formatting is kept in markdown I'm all ears. I tried a couple things (<pre>, unicode em space), but this was the only method that worked consistently for me with vscode's markdown renderer

Youssef1313 commented 3 years ago

@dibarbet Alright, for this case it looks like it's needed. However, in other places (middle of words) the extra spaces should usually be trimmed. This is done intentionally in VS IntelliSense:

https://github.com/dotnet/roslyn/blob/acf3e276f7d7330274ea537a8b4887b4e2e7f544/src/Features/Core/Portable/DocumentationComments/AbstractDocumentationCommentFormattingService.cs#L550

For example:

<summary>Text    with         many           spaces</summary>

should render as Text with many spaces. Spaces should be normalized even in <c>. The only place it shouldn't be normalized is in <code>

dibarbet commented 3 years ago

Ah interesting! Perhaps the the &nbsp; should only apply to lines with leading space outside of fences. Markdown would automatically trim spaces elsewhere even inside these code fences, but not in

 these      code      fences

which sounds like the behavior we want.

dibarbet commented 3 years ago

Let me know if you wanted to work on this issue, otherwise I'll assign it to myself to fix.

Youssef1313 commented 3 years ago

@dibarbet I'll take a look after https://github.com/dotnet/roslyn/pull/53140 goes in (this PR adds TaggedTextStyle.InlineCode which would be needed to differentiate <code> and <c>.

sharwell commented 3 years ago

Markdown ignores normal leading spaces, so the following written in markdown

Exceptions:
    XYZ

renders as the below Exceptions: XYZ

...

If you have a better method to ensure the whitespace formatting is kept in markdown I'm all ears. ...

The rendering without leading whitespace is the correct rendering, so there is no need to try and add spaces.

dibarbet commented 3 years ago

@sharwell at least in 16.9, it shows with indentation which is what I was going off of image

sharwell commented 3 years ago

I would use headings in markdown instead:

### Returns

something

### Exceptions

`NullReferenceException`
dibarbet commented 3 years ago

Hmm, that's a good idea. Works for me!