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.

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

Improve Hover markdown on 'await' keyword #70629

Merged
merged 3 commits into from
Nov 7, 2023
Merged

Conversation

dibarbet
Copy link
Member

Improve the appearance of hover on await with return by using an inline code fence.
Resolves dotnet/vscode-csharp#6577

This is not perfect - unfortunately VSCode / markdown doesn't allow us to specify a language name for inline code fences, so it is not colorized. The alternative is to modify this to be a code block on a new line, but that doesn't look great either. This approach seemed reasonable to me.

Now looks like
image

@dibarbet dibarbet requested a review from a team as a code owner October 30, 2023 20:54
@dotnet-issue-labeler dotnet-issue-labeler bot added Area-IDE untriaged Issues and PRs which have not yet been triaged by a lead labels Oct 30, 2023
@@ -844,40 +846,65 @@ public static LSP.MarkupContent GetDocumentationMarkupContent(ImmutableArray<Tag
};
}

var builder = new StringBuilder();
var isInCodeBlock = false;
using var markdownBuilder = new MarkdownContentBuilder();
Copy link
Member Author

Choose a reason for hiding this comment

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

switched to a more manual builder so I can access the contents of lines

_linesBuilder = ArrayBuilder<string>.GetInstance();
}

public void Append(string text)
Copy link
Member

Choose a reason for hiding this comment

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

Could text every contain embedded newlines?

Copy link
Member Author

Choose a reason for hiding this comment

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

Ideally it wouldn't be called like that, but I'm also not sure how to enforce that.

@CyrusNajmabadi
Copy link
Member

What about "returns System.String"? It's class really needed?

@dibarbet
Copy link
Member Author

What about "returns System.String"? It's class really needed?

So how this works is it actually just wraps the main description info from the symbol being returned. So it has 'class' in there because it just wraps the main description for string:
image

Unfortunately I don't think there is an easy way to remove 'class' without changing where we acquire the text for the return type. So it'd be a bit more of an involved change.

@dibarbet
Copy link
Member Author

dibarbet commented Nov 6, 2023

/azp run roslyn-integration-CI

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@CyrusNajmabadi
Copy link
Member

This is not perfect - unfortunately VSCode / markdown doesn't allow us to specify a language name for inline code fences

Let's open a request on htat as well :)

@dibarbet dibarbet merged commit bd61df4 into dotnet:main Nov 7, 2023
24 checks passed
@dibarbet dibarbet deleted the await_hover branch November 7, 2023 19:40
@ghost ghost added this to the Next milestone Nov 7, 2023
@DoctorKrolic
Copy link
Contributor

unfortunately VSCode / markdown doesn't allow us to specify a language name for inline code fences

TBH the very idea of specifying a language as a markdown parameter is agains the whole language server idea since it requires client to know what language it is and how to colorize it. It would be perfect if LSP protocol allowed arbitrary colorizations and structuring in hovers similar to VS's hover API (colorized text runs and so on), but considering the speed of LSP development (or its absence) this will probably be added only in a very distant future(

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-IDE untriaged Issues and PRs which have not yet been triaged by a lead
Projects
None yet
Development

Successfully merging this pull request may close these issues.

The format of await quick info content is wrong
5 participants