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.

Sign up for GitHub

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

Be opinionated about the formatting of section blocks #9252

Merged
merged 1 commit into from
Sep 9, 2023
Merged

Be opinionated about the formatting of section blocks #9252

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 21 additions & 7 deletions ...azor/src/Microsoft.AspNetCore.Razor.LanguageServer/Formatting/CSharpFormattingPassBase.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -363,7 +363,7 @@ private static bool ShouldFormatCore(FormattingContext context, TextSpan mapping
return false;
}

if (IsInSectionDirectiveCloseBrace())
if (IsInSectionDirectiveBrace())
{
return false;
}
Expand Down Expand Up @@ -516,19 +516,33 @@ bool IsInTemplateBlock()
return owner.AncestorsAndSelf().Any(n => n is CSharpTemplateBlockSyntax);
}

bool IsInSectionDirectiveCloseBrace()
bool IsInSectionDirectiveBrace()
{
// @section Scripts {
// <script></script>
// }
//
// We are fine to format these, but due to how they are generated (inside a multi-line lambda)
// we want to exlude the final close brace from being formatted, or it will be indented by one
// level due to the lambda. The rest we don't need to worry about, because the one level indent
// is actually desirable.
// Due to how sections are generated (inside a multi-line lambda), we want to exclude the braces
// from being formatted, or it will be indented by one level due to the lambda. The rest we don't
// need to worry about, because the one level indent is actually desirable.

// Due to the Razor tree being so odd, the checks for open and close are surprisingly different

// Open brace is a child of the C# code block that is the directive itself
if (owner is RazorMetaCodeSyntax &&
owner.Parent is CSharpCodeBlockSyntax codeBlock &&
owner == codeBlock.Children[3] &&
// CSharpCodeBlock -> RazorDirectiveBody -> RazorDirective
codeBlock.Parent?.Parent is RazorDirectiveSyntax directive2 &&
directive2.DirectiveDescriptor.Directive == SectionDirective.Directive.Directive)
{
return true;
}

// Close brace is a child of the section content, which is a MarkupBlock
if (owner is MarkupTextLiteralSyntax &&
owner.Parent is MarkupBlockSyntax block &&
owner == block.Children[block.Children.Count - 1] &&
owner == block.Children[^1] &&
// MarkupBlock -> CSharpCodeBlock -> RazorDirectiveBody -> RazorDirective
block.Parent?.Parent?.Parent is RazorDirectiveSyntax directive &&
directive.DirectiveDescriptor.Directive == SectionDirective.Directive.Directive)
Expand Down
75 changes: 70 additions & 5 deletions src/Razor/src/Microsoft.AspNetCore.Razor.LanguageServer/Formatting/RazorFormattingPass.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
using System.Threading;
using System.Threading.Tasks;
using Microsoft.AspNetCore.Razor.Language;
using Microsoft.AspNetCore.Razor.Language.Extensions;
using Microsoft.AspNetCore.Razor.Language.Syntax;
using Microsoft.AspNetCore.Razor.LanguageServer.Extensions;
using Microsoft.Extensions.Logging;
Expand Down Expand Up @@ -90,12 +91,76 @@ private static IEnumerable<TextEdit> FormatRazor(FormattingContext context, Razo
return edits;
}

private static bool TryFormatBlocks(FormattingContext context, IList<TextEdit> edits, RazorSourceDocument source, SyntaxNode node)
private static void TryFormatBlocks(FormattingContext context, List<TextEdit> edits, RazorSourceDocument source, SyntaxNode node)
{
return TryFormatFunctionsBlock(context, edits, source, node) ||
// We only want to run one of these
_ = TryFormatFunctionsBlock(context, edits, source, node) ||
TryFormatCSharpExplicitTransition(context, edits, source, node) ||
TryFormatHtmlInCSharp(context, edits, source, node) ||
TryFormatComplexCSharpBlock(context, edits, source, node);
TryFormatComplexCSharpBlock(context, edits, source, node) ||
TryFormatSectionBlock(context, edits, source, node);
}

private static bool TryFormatSectionBlock(FormattingContext context, List<TextEdit> edits, RazorSourceDocument source, SyntaxNode node)
{
// @section Goo {
// }
//
// or
//
// @section Goo
// {
// }
if (node is CSharpCodeBlockSyntax directiveCode &&
directiveCode.Children is [RazorDirectiveSyntax directive] &&
directive.DirectiveDescriptor?.Directive == SectionDirective.Directive.Directive &&
directive.Body is RazorDirectiveBodySyntax { CSharpCode: { } code })
{
var children = code.Children;
if (TryGetWhitespace(children, out var whitespaceBeforeSectionName, out var whitespaceAfterSectionName))
{
// For whitespace we normalize it differently depending on if its multi-line or not
FormatWhitespaceBetweenDirectiveAndBrace(whitespaceBeforeSectionName, directive, edits, source, context);
FormatWhitespaceBetweenDirectiveAndBrace(whitespaceAfterSectionName, directive, edits, source, context);

return true;
}
else if (children.TryGetOpenBraceToken(out var brace))
{
// If there is no whitespace at all we normalize to a single space
var start = brace.GetRange(source).Start;
var edit = new TextEdit
{
Range = new Range { Start = start, End = start },
NewText = " "
};
edits.Add(edit);

return true;
}
}

return false;

static bool TryGetWhitespace(SyntaxList<RazorSyntaxNode> children, [NotNullWhen(true)] out CSharpStatementLiteralSyntax? whitespaceBeforeSectionName, [NotNullWhen(true)] out UnclassifiedTextLiteralSyntax? whitespaceAfterSectionName)
{
// If there is whitespace between the directive and the section name, and the section name and the brace, they will be in the first child
// and third child of the 6 total children
whitespaceBeforeSectionName = null;
whitespaceAfterSectionName = null;
if (children.Count == 6 &&
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems hacky but that's probably just the Razor formatter in general 😩

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, pretty much. It's more of a comment on the syntax tree really. In a future world this would be a SectionBlockSyntax or something, so we'd know 100% what we're doing, instead of using these odd heuristics

children[0] is CSharpStatementLiteralSyntax before &&
before.ContainsOnlyWhitespace() &&
children[2] is UnclassifiedTextLiteralSyntax after &&
after.ContainsOnlyWhitespace())
{
whitespaceBeforeSectionName = before;
whitespaceAfterSectionName = after;

}

return whitespaceBeforeSectionName != null;
}
}

private static bool TryFormatFunctionsBlock(FormattingContext context, IList<TextEdit> edits, RazorSourceDocument source, SyntaxNode node)
Expand Down Expand Up @@ -140,8 +205,8 @@ private static bool TryFormatCSharpExplicitTransition(FormattingContext context,
// @{
// var x = 1;
// }
if (node is CSharpCodeBlockSyntax expliciteCode &&
expliciteCode.Children.FirstOrDefault() is CSharpStatementSyntax statement &&
if (node is CSharpCodeBlockSyntax explicitCode &&
explicitCode.Children.FirstOrDefault() is CSharpStatementSyntax statement &&
statement.Body is CSharpStatementBodySyntax csharpStatementBody)
{
var openBraceNode = csharpStatementBody.OpenBrace;
Expand Down
46 changes: 46 additions & 0 deletions .../Microsoft.AspNetCore.Razor.LanguageServer.Test/Formatting/CodeDirectiveFormattingTest.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -510,6 +510,52 @@ @section Scripts {
fileKind: FileKinds.Legacy);
}

[Fact]
public async Task Format_SectionDirectiveBlock7()
{
await RunFormattingTestAsync(
input: """
@functions {
public class Foo{
void Method() { }
}
}

@section Scripts
{
<meta property="a" content="b">
<meta property="a" content="b"/>
<meta property="a" content="b">

@if(true)
{
<p>this is a paragraph</p>
}
}
""",
expected: """
@functions {
public class Foo
{
void Method() { }
}
}

@section Scripts
{
<meta property="a" content="b">
<meta property="a" content="b" />
<meta property="a" content="b">

@if (true)
{
<p>this is a paragraph</p>
}
}
""",
fileKind: FileKinds.Legacy);
}

[Fact]
public async Task Formats_CodeBlockDirectiveWithRazorComments()
{
Expand Down
36 changes: 36 additions & 0 deletions ...zor/test/Microsoft.AspNetCore.Razor.LanguageServer.Test/Formatting/RazorFormattingTest.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,42 @@ public RazorFormattingTest(ITestOutputHelper testOutput)
{
}

[Fact]
public async Task Section_BraceOnNextLine()
{
await RunFormattingTestAsync(
input: """
@section Scripts
{
<meta property="a" content="b">
}
""",
expected: """
@section Scripts
{
<meta property="a" content="b">
}
""",
fileKind: FileKinds.Legacy);
}

[Fact]
public async Task Section_BraceOnSameLine()
{
await RunFormattingTestAsync(
input: """
@section Scripts {
<meta property="a" content="b">
}
""",
expected: """
@section Scripts {
<meta property="a" content="b">
}
""",
fileKind: FileKinds.Legacy);
}

[Fact]
public async Task CodeBlock_SpansMultipleLines()
{
Expand Down