From 747689e481aed4b3ea24d3937f726cb83c9f7a28 Mon Sep 17 00:00:00 2001 From: Fredric Silberberg Date: Tue, 18 Aug 2020 18:14:00 -0700 Subject: [PATCH] Ensure that all quickinfo sections have linebreaks between them, and don't add unecessary duplicate linebreaks. --- .../Helpers/MarkdownHelpers.cs | 11 +++++++- .../Services/Completion/CompletionService.cs | 2 +- .../Services/QuickInfoProvider.cs | 18 +++++------- .../QuickInfoProviderFacts.cs | 28 ++++++++++++++++--- 4 files changed, 42 insertions(+), 17 deletions(-) diff --git a/src/OmniSharp.Roslyn.CSharp/Helpers/MarkdownHelpers.cs b/src/OmniSharp.Roslyn.CSharp/Helpers/MarkdownHelpers.cs index 56392a366c..58c70fd484 100644 --- a/src/OmniSharp.Roslyn.CSharp/Helpers/MarkdownHelpers.cs +++ b/src/OmniSharp.Roslyn.CSharp/Helpers/MarkdownHelpers.cs @@ -33,11 +33,18 @@ public static string Escape(string markdown) /// private const string ContainerEnd = nameof(ContainerEnd); + public static bool StartsWithNewline(this ImmutableArray taggedParts) + { + return !taggedParts.IsDefaultOrEmpty + && taggedParts[0].Tag switch { TextTags.LineBreak => true, ContainerStart => true, _ => false }; + } + public static void TaggedTextToMarkdown( ImmutableArray taggedParts, StringBuilder stringBuilder, FormattingOptions formattingOptions, - MarkdownFormat markdownFormat) + MarkdownFormat markdownFormat, + out bool endedWithLineBreak) { bool isInCodeBlock = false; bool brokeLine = true; @@ -136,6 +143,7 @@ public static void TaggedTextToMarkdown( Debug.Assert(i == taggedParts.Length); stringBuilder.Append(formattingOptions.NewLine); stringBuilder.Append("```"); + endedWithLineBreak = false; return; } } @@ -204,6 +212,7 @@ public static void TaggedTextToMarkdown( stringBuilder.Append("_"); } + endedWithLineBreak = brokeLine; return; void addText(string text) diff --git a/src/OmniSharp.Roslyn.CSharp/Services/Completion/CompletionService.cs b/src/OmniSharp.Roslyn.CSharp/Services/Completion/CompletionService.cs index 7d89f6c69e..b1c481f901 100644 --- a/src/OmniSharp.Roslyn.CSharp/Services/Completion/CompletionService.cs +++ b/src/OmniSharp.Roslyn.CSharp/Services/Completion/CompletionService.cs @@ -429,7 +429,7 @@ public async Task Handle(CompletionResolveRequest req var description = await completionService.GetDescriptionAsync(document, lastCompletionItem); StringBuilder textBuilder = new StringBuilder(); - MarkdownHelpers.TaggedTextToMarkdown(description.TaggedParts, textBuilder, _formattingOptions, MarkdownFormat.FirstLineAsCSharp); + MarkdownHelpers.TaggedTextToMarkdown(description.TaggedParts, textBuilder, _formattingOptions, MarkdownFormat.FirstLineAsCSharp, out _); request.Item.Documentation = textBuilder.ToString(); diff --git a/src/OmniSharp.Roslyn.CSharp/Services/QuickInfoProvider.cs b/src/OmniSharp.Roslyn.CSharp/Services/QuickInfoProvider.cs index 30587d1f39..95675aeffe 100644 --- a/src/OmniSharp.Roslyn.CSharp/Services/QuickInfoProvider.cs +++ b/src/OmniSharp.Roslyn.CSharp/Services/QuickInfoProvider.cs @@ -4,7 +4,6 @@ using System.Threading.Tasks; using Microsoft.CodeAnalysis; using Microsoft.CodeAnalysis.QuickInfo; -using Microsoft.CodeAnalysis.Text; using Microsoft.Extensions.Logging; using OmniSharp.Extensions; using OmniSharp.Mef; @@ -71,19 +70,11 @@ public async Task Handle(QuickInfoRequest request) var finalTextBuilder = new StringBuilder(); + bool lastSectionHadLineBreak = true; var description = quickInfo.Sections.FirstOrDefault(s => s.Kind == QuickInfoSectionKinds.Description); if (description is object) { appendSection(description, MarkdownFormat.AllTextAsCSharp); - - // The description doesn't include a set of newlines at the end, regardless - // of whether there are more sections, so if there are more sections we need - // to ensure they're separated. - if (quickInfo.Sections.Length > 1) - { - finalTextBuilder.Append(_formattingOptions.NewLine); - finalTextBuilder.Append(_formattingOptions.NewLine); - } } var summary = quickInfo.Sections.FirstOrDefault(s => s.Kind == QuickInfoSectionKinds.DocumentationComments); @@ -127,7 +118,12 @@ public async Task Handle(QuickInfoRequest request) void appendSection(QuickInfoSection section, MarkdownFormat format) { - MarkdownHelpers.TaggedTextToMarkdown(section.TaggedParts, finalTextBuilder, _formattingOptions, format); + if (!lastSectionHadLineBreak && !section.TaggedParts.StartsWithNewline()) + { + finalTextBuilder.Append(_formattingOptions.NewLine); + finalTextBuilder.Append(_formattingOptions.NewLine); + } + MarkdownHelpers.TaggedTextToMarkdown(section.TaggedParts, finalTextBuilder, _formattingOptions, format, out lastSectionHadLineBreak); } } } diff --git a/tests/OmniSharp.Roslyn.CSharp.Tests/QuickInfoProviderFacts.cs b/tests/OmniSharp.Roslyn.CSharp.Tests/QuickInfoProviderFacts.cs index cc386326a9..5f1757b2a7 100644 --- a/tests/OmniSharp.Roslyn.CSharp.Tests/QuickInfoProviderFacts.cs +++ b/tests/OmniSharp.Roslyn.CSharp.Tests/QuickInfoProviderFacts.cs @@ -230,7 +230,7 @@ public async Task DisplayFormatFor_TypeSymbol_ComplexType_DifferentNamespace() public async Task DisplayFormatFor_TypeSymbol_WithGenerics() { var response = await GetTypeLookUpResponse(line: 15, column: 36); - Assert.Equal("```csharp\ninterface System.Collections.Generic.IDictionary\n```\n\n\n\n```csharp\nTKey is string\nTValue is IEnumerable\n```", response.Markdown); + Assert.Equal("```csharp\ninterface System.Collections.Generic.IDictionary\n```\n\n```csharp\nTKey is string\nTValue is IEnumerable\n```", response.Markdown); } [Fact] @@ -338,7 +338,7 @@ class testissue } }"; var response = await GetTypeLookUpResponse(content); - Assert.Equal("```csharp\nbool testissue.Compare(int gameObject, string tagName)\n```\n\n\n\nReturns:\n\n Returns true if object is tagged with tag\\.", response.Markdown); + Assert.Equal("```csharp\nbool testissue.Compare(int gameObject, string tagName)\n```\n\nReturns:\n\n Returns true if object is tagged with tag\\.", response.Markdown); } [Fact] @@ -369,7 +369,7 @@ class testissue } }"; var response = await GetTypeLookUpResponse(content); - Assert.Equal("```csharp\nbool testissue.Compare(int gameObject, string tagName)\n```\n\n\n\nExceptions:\n\n A\n\n B", response.Markdown); + Assert.Equal("```csharp\nbool testissue.Compare(int gameObject, string tagName)\n```\n\nExceptions:\n\n A\n\n B", response.Markdown); } [Fact] @@ -665,7 +665,7 @@ void M2() } }"; var response = await GetTypeLookUpResponse(content); - Assert.Equal("```csharp\nvoid C.M1<'a>('a t)\n```\n\n\n\nAnonymous Types:\n\n```csharp\n 'a is new { int X, int Y }\n```", response.Markdown); + Assert.Equal("```csharp\nvoid C.M1<'a>('a t)\n```\n\nAnonymous Types:\n\n```csharp\n 'a is new { int X, int Y }\n```", response.Markdown); } [Fact] @@ -726,6 +726,26 @@ public static void A(string s) Assert.Equal("```csharp\n(parameter) string s\n```\n\n_'s' is not null here\\._", response.Markdown); } + [Fact] + public async Task NullableFieldWithComments() + { + string content = @" +#nullable enable +class Program +{ + /// Interesting content. + public string? _s; + + public static void A() + { + _ = _s$$; + } + +}"; + var response = await GetTypeLookUpResponse(content); + Assert.Equal("```csharp\n(field) string? Program._s\n```\n\nInteresting content\\.\n\n_'\\_s' is not null here\\._", response.Markdown); + } + private async Task GetTypeLookUpResponse(string content) { TestFile testFile = new TestFile("dummy.cs", content);