Skip to content

Commit

Permalink
Avoid putting URLs into 'code' field in diagnostics (#14583)
Browse files Browse the repository at this point in the history 
Closes #14570

Before:
<img width="1020" alt="image"
src="https://github.com/user-attachments/assets/f0d528ef-50c5-439a-8c83-6485644239b1">

After (clicking the code takes you to the right link):
<img width="828" alt="image"
src="https://github.com/user-attachments/assets/5a74aadf-32c5-40d7-a56e-61758021bd8a">

###### Microsoft Reviewers: [Open in
CodeFlow](https://microsoft.github.io/open-pr/?codeflow=https://github.com/Azure/bicep/pull/14583)
  • Loading branch information
@anthony-c-martin
anthony-c-martin authored Jul 18, 2024
1 parent 684e6d4 commit d5c6d14
Show file tree
Hide file tree
Showing 5 changed files with 52 additions and 57 deletions.
15 changes: 15 additions & 0 deletions src/Bicep.LangServer.IntegrationTests/Assertions/OmniSharpDiagnosticAssertions.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -42,5 +42,20 @@ public AndConstraint<OmniSharpDiagnosticAssertions> HaveCodeAndSeverity(string c

return new AndConstraint<OmniSharpDiagnosticAssertions>(this);
}

public AndConstraint<OmniSharpDiagnosticAssertions> HaveDocumentationUrl(Uri docsUrl, string because = "", params object[] becauseArgs)
{
Execute.Assertion
.BecauseOf(because, becauseArgs)
.Given(() => Subject.Message)
.ForCondition(x => !string.IsNullOrWhiteSpace(x))
.FailWith("Expected message to have content but it was '{0}'", m => m)
.Then
.Given<CodeDescription?>(_ => Subject.CodeDescription)
.ForCondition(x => x?.Href == docsUrl)
.FailWith("Expected URL to be '{0}' but it was '{1}'", _ => docsUrl, x => x?.Href);

return new AndConstraint<OmniSharpDiagnosticAssertions>(this);
}
}
}
30 changes: 15 additions & 15 deletions src/Bicep.LangServer.IntegrationTests/BicepConfigTests.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@ await VerifyDiagnosticsAsync(diagsListener, mainUri, (@"Parameter ""storageAccou
DiagnosticSeverity.Information,
new Position(0, 6),
new Position(0, 24),
"https://aka.ms/bicep/linter/no-unused-params"));
"no-unused-params"));
}

// update bicepconfig.json and verify diagnostics message is not sent
Expand Down Expand Up @@ -113,7 +113,7 @@ await VerifyDiagnosticsAsync(diagsListener, mainUri, (@"Parameter ""storageAccou
DiagnosticSeverity.Information,
new Position(0, 6),
new Position(0, 24),
"https://aka.ms/bicep/linter/no-unused-params"));
"no-unused-params"));
}

// Delete bicepconfig.json and verify diagnostics are based off of default bicepconfig.json
Expand All @@ -133,7 +133,7 @@ await VerifyDiagnosticsAsync(diagsListener, mainUri, (@"Parameter ""storageAccou
DiagnosticSeverity.Warning,
new Position(0, 6),
new Position(0, 24),
"https://aka.ms/bicep/linter/no-unused-params"));
"no-unused-params"));
}
}

Expand All @@ -155,7 +155,7 @@ await VerifyDiagnosticsAsync(diagsListener, mainUri, (@"Parameter ""storageAccou
DiagnosticSeverity.Warning,
new Position(0, 6),
new Position(0, 24),
"https://aka.ms/bicep/linter/no-unused-params"));
"no-unused-params"));
}

// Create bicepconfig.json and verify diagnostics
Expand Down Expand Up @@ -188,7 +188,7 @@ await VerifyDiagnosticsAsync(diagsListener, mainUri, (@"Parameter ""storageAccou
DiagnosticSeverity.Information,
new Position(0, 6),
new Position(0, 24),
"https://aka.ms/bicep/linter/no-unused-params"));
"no-unused-params"));
}
}

Expand All @@ -210,7 +210,7 @@ await VerifyDiagnosticsAsync(diagsListener, mainUri, (@"Parameter ""storageAccou
DiagnosticSeverity.Warning,
new Position(0, 6),
new Position(0, 24),
"https://aka.ms/bicep/linter/no-unused-params"));
"no-unused-params"));
}

// Create bicepconfig.json and verify diagnostics
Expand Down Expand Up @@ -244,7 +244,7 @@ await VerifyDiagnosticsAsync(diagsListener, mainUri, (@"Parameter ""storageAccou
DiagnosticSeverity.Information,
new Position(0, 6),
new Position(0, 24),
"https://aka.ms/bicep/linter/no-unused-params"));
"no-unused-params"));
}
}

Expand Down Expand Up @@ -283,7 +283,7 @@ await VerifyDiagnosticsAsync(diagsListener, mainUri, (@"Parameter ""storageAccou
DiagnosticSeverity.Information,
new Position(0, 6),
new Position(0, 24),
"https://aka.ms/bicep/linter/no-unused-params"));
"no-unused-params"));
}

// create new bicepconfig.json and verify diagnostics
Expand Down Expand Up @@ -317,7 +317,7 @@ await VerifyDiagnosticsAsync(diagsListener, mainUri, (@"Parameter ""storageAccou
DiagnosticSeverity.Warning,
new Position(0, 6),
new Position(0, 24),
"https://aka.ms/bicep/linter/no-unused-params"));
"no-unused-params"));
}
}

Expand Down Expand Up @@ -355,7 +355,7 @@ await VerifyDiagnosticsAsync(diagsListener, mainUri, (@"Parameter ""storageAccou
DiagnosticSeverity.Information,
new Position(0, 6),
new Position(0, 24),
"https://aka.ms/bicep/linter/no-unused-params"));
"no-unused-params"));
}

// add bicepconfig.json to parent directory and verify diagnostics
Expand Down Expand Up @@ -388,7 +388,7 @@ await VerifyDiagnosticsAsync(diagsListener, mainUri, (@"Parameter ""storageAccou
DiagnosticSeverity.Information,
new Position(0, 6),
new Position(0, 24),
"https://aka.ms/bicep/linter/no-unused-params"));
"no-unused-params"));
}
}

Expand Down Expand Up @@ -427,7 +427,7 @@ await VerifyDiagnosticsAsync(diagsListener,
DiagnosticSeverity.Warning,
new Position(0, 6),
new Position(0, 24),
"https://aka.ms/bicep/linter/no-unused-params"));
"no-unused-params"));
}

// update bicepconfig.json and verify diagnostics
Expand Down Expand Up @@ -458,7 +458,7 @@ await VerifyDiagnosticsAsync(diagsListener, mainUri, (@"Parameter ""storageAccou
DiagnosticSeverity.Warning,
new Position(0, 6),
new Position(0, 24),
"https://aka.ms/bicep/linter/no-unused-params"));
"no-unused-params"));
}
}

Expand Down Expand Up @@ -498,7 +498,7 @@ await VerifyDiagnosticsAsync(diagsListener, uriOfFileInChildDirectory, (@"Parame
DiagnosticSeverity.Information,
new Position(0, 6),
new Position(0, 24),
"https://aka.ms/bicep/linter/no-unused-params"));
"no-unused-params"));
}

// add bicepconfig.json to parent directory and verify diagnostics
Expand Down Expand Up @@ -536,7 +536,7 @@ await VerifyDiagnosticsAsync(diagsListener, uriOfFileInParentDirectory, (@"Param
DiagnosticSeverity.Warning,
new Position(0, 6),
new Position(0, 24),
"https://aka.ms/bicep/linter/no-unused-params"));
"no-unused-params"));
}
}

Expand Down
6 changes: 3 additions & 3 deletions src/Bicep.LangServer.IntegrationTests/TextDocumentSyncTests.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -46,8 +46,8 @@ public async Task DidOpenTextDocument_should_trigger_PublishDiagnostics()
d =>
{
d.Range.Should().HaveRange((1, 6), (1, 13));
// note documentation pretty printing moves Uri to code for output
d.Should().HaveCodeAndSeverity(new NoUnusedParametersRule().Uri!.AbsoluteUri, DiagnosticSeverity.Warning);
d.Should().HaveCodeAndSeverity(NoUnusedParametersRule.Code, DiagnosticSeverity.Warning)
.And.HaveDocumentationUrl(new NoUnusedParametersRule().Uri!);
},
d =>
{
Expand Down Expand Up @@ -81,7 +81,7 @@ public async Task DidOpenTextDocument_should_trigger_PublishDiagnostics()
{
d.Range.Should().HaveRange((1, 6), (1, 13));
// documentation provided with linter sets code to uri for pretty link print outs
d.Should().HaveCodeAndSeverity(new NoUnusedParametersRule().Uri!.AbsoluteUri, DiagnosticSeverity.Warning);
d.Should().HaveCodeAndSeverity(NoUnusedParametersRule.Code, DiagnosticSeverity.Warning);
},
d =>
{
Expand Down
8 changes: 4 additions & 4 deletions src/Bicep.LangServer.UnitTests/Configuration/BicepConfigChangeHandlerTests.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,7 @@ public void RefreshCompilationOfSourceFilesInWorkspace_WithValidBicepConfigFile_
{
x.Message.Should().Be(@"Parameter ""storageAccountName"" is declared but never used.");
x.Severity.Should().Be(DiagnosticSeverity.Information);
x.Code?.String.Should().Be("https://aka.ms/bicep/linter/no-unused-params");
x.Code?.String.Should().Be("no-unused-params");
x.Range.Should().Be(new Range
{
Start = new Position(0, 6),
Expand Down Expand Up @@ -111,7 +111,7 @@ public void RefreshCompilationOfSourceFilesInWorkspace_WithInvalidBicepConfigFil
{
x.Message.Should().Be(@"Parameter ""storageAccountName"" is declared but never used.");
x.Severity.Should().Be(DiagnosticSeverity.Warning);
x.Code?.String.Should().Be("https://aka.ms/bicep/linter/no-unused-params");
x.Code?.String.Should().Be("no-unused-params");
x.Range.Should().Be(new Range
{
Start = new Position(0, 6),
Expand Down Expand Up @@ -154,7 +154,7 @@ public void RefreshCompilationOfSourceFilesInWorkspace_WithBicepConfigFileThatDo
{
x.Message.Should().Be(@"Parameter ""storageAccountName"" is declared but never used.");
x.Severity.Should().Be(DiagnosticSeverity.Warning);
x.Code?.String.Should().Be("https://aka.ms/bicep/linter/no-unused-params");
x.Code?.String.Should().Be("no-unused-params");
x.Range.Should().Be(new Range
{
Start = new Position(0, 6),
Expand Down Expand Up @@ -210,7 +210,7 @@ public void RefreshCompilationOfSourceFilesInWorkspace_WithoutBicepConfigFile_Sh
{
x.Message.Should().Be(@"Parameter ""storageAccountName"" is declared but never used.");
x.Severity.Should().Be(DiagnosticSeverity.Warning);
x.Code?.String.Should().Be("https://aka.ms/bicep/linter/no-unused-params");
x.Code?.String.Should().Be("no-unused-params");
x.Range.Should().Be(new Range
{
Start = new Position(0, 6),
Expand Down
50 changes: 15 additions & 35 deletions src/Bicep.LangServer/Extensions/DiagnosticExtensions.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,58 +3,38 @@
using System.Collections.Immutable;
using Bicep.Core.Diagnostics;
using OmniSharp.Extensions.LanguageServer.Protocol.Models;
using LspDiagnostic = OmniSharp.Extensions.LanguageServer.Protocol.Models.Diagnostic;

namespace Bicep.LanguageServer.Extensions
{
public static class DiagnosticExtensions
{
public static IEnumerable<OmniSharp.Extensions.LanguageServer.Protocol.Models.Diagnostic> ToDiagnostics(this IEnumerable<IDiagnostic> source, ImmutableArray<int> lineStarts)
public static IEnumerable<LspDiagnostic> ToDiagnostics(this IEnumerable<IDiagnostic> source, ImmutableArray<int> lineStarts)
=> source.Select(diagnostic => CreateDiagnostic(diagnostic, lineStarts));

private static OmniSharp.Extensions.LanguageServer.Protocol.Models.Diagnostic CreateDiagnostic(IDiagnostic diagnostic, ImmutableArray<int> lineStarts)
{
var (descCode, codeDescription) = GetDiagnosticDocumentation(diagnostic);
return new OmniSharp.Extensions.LanguageServer.Protocol.Models.Diagnostic()
private static LspDiagnostic CreateDiagnostic(IDiagnostic diagnostic, ImmutableArray<int> lineStarts)
=> new()
{
Severity = ToDiagnosticSeverity(diagnostic.Level),
Code = descCode ?? diagnostic.Code,
Code = diagnostic.Code,
Message = diagnostic.Message,
Source = diagnostic.Source,
Range = diagnostic.ToRange(lineStarts),
Tags = ToDiagnosticTags(diagnostic.Styling),
CodeDescription = codeDescription
CodeDescription = GetDiagnosticDocumentation(diagnostic),
};
}

private static (string?, CodeDescription?) GetDiagnosticDocumentation(IDiagnostic diagnostic)
{
if (diagnostic.Uri != null)
{
// This shuffling of the Code to Uri gives us the message formatting
// that is desired where the documentation link is displayed as the text
// of the link. Otherwise the code is displayed rather than the Uri
//
// Default message format:
// Declared parameter must be referenced within the document scope. bicep core(no-unused-params) [2,7]
//
// Desired format:
// Declared parameter must be referenced within the document scope. bicep core(https://aka.ms/bicep/linter/no-unused-params) [2,7]

return new(diagnostic.Uri.AbsoluteUri, new CodeDescription() { Href = diagnostic.Uri });
}

// no additional documentation
return new(null, null);
}
private static CodeDescription? GetDiagnosticDocumentation(IDiagnostic diagnostic)
=> diagnostic.Uri is { } ? new() { Href = diagnostic.Uri } : null;

private static DiagnosticSeverity ToDiagnosticSeverity(DiagnosticLevel level)
=> level switch
{
DiagnosticLevel.Info => DiagnosticSeverity.Information,
DiagnosticLevel.Warning => DiagnosticSeverity.Warning,
DiagnosticLevel.Error => DiagnosticSeverity.Error,
_ => throw new ArgumentException($"Unrecognized level {level}"),
};
=> level switch
{
DiagnosticLevel.Info => DiagnosticSeverity.Information,
DiagnosticLevel.Warning => DiagnosticSeverity.Warning,
DiagnosticLevel.Error => DiagnosticSeverity.Error,
_ => throw new ArgumentException($"Unrecognized level {level}"),
};

private static Container<DiagnosticTag>? ToDiagnosticTags(DiagnosticStyling styling) => styling switch
{
Expand Down

0 comments on commit d5c6d14

Please sign in to comment.