Skip to content

Fix formatting of lines that start with component attributes #8279

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

Merged
merged 2 commits into from
Feb 21, 2023
Merged

Fix formatting of lines that start with component attributes #8279

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
9133a8f
Fix formatting of lines that start with component attributes
davidwengier Feb 15, 2023
3a974cc
Fix some cases of formatting component attributes inside render fragm…
davidwengier Feb 15, 2023
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
37 changes: 30 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 @@ -167,7 +167,7 @@ protected async Task<List<TextChange>> AdjustIndentationAsync(FormattingContext
var line = context.SourceText.Lines[i];
var lineStart = line.GetFirstNonWhitespacePosition() ?? line.Start;
var lineStartSpan = new TextSpan(lineStart, 0);
if (!ShouldFormat(context, lineStartSpan, allowImplicitStatements: true))
if (!ShouldFormatLine(context, lineStartSpan, allowImplicitStatements: true))
Copy link
Member Author

@davidwengier davidwengier Feb 15, 2023

Choose a reason for hiding this comment

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

This ShouldFormat method is arguably still doing too much, but this particular usage has annoyed me before, so I done fixed it.

{
// We don't care about this line as it lies in an area we don't want to format.
continue;
Expand Down Expand Up @@ -279,6 +279,16 @@ protected static bool ShouldFormat(FormattingContext context, TextSpan mappingSp
}

protected static bool ShouldFormat(FormattingContext context, TextSpan mappingSpan, bool allowImplicitStatements, out SyntaxNode? foundOwner)
{
return ShouldFormatCore(context, mappingSpan, allowImplicitStatements, isLineRequest: false, out foundOwner);
}

private static bool ShouldFormatLine(FormattingContext context, TextSpan mappingSpan, bool allowImplicitStatements)
{
return ShouldFormatCore(context, mappingSpan, allowImplicitStatements, isLineRequest: true, out _);
}

private static bool ShouldFormatCore(FormattingContext context, TextSpan mappingSpan, bool allowImplicitStatements, bool isLineRequest, out SyntaxNode? foundOwner)
{
// We should be called with the range of various C# SourceMappings.

Expand Down Expand Up @@ -323,7 +333,7 @@ protected static bool ShouldFormat(FormattingContext context, TextSpan mappingSp
}

if (IsRazorComment() ||
IsInHtmlAttributeName() ||
IsInBoundComponentAttributeName() ||
IsInHtmlAttributeValue() ||
IsInDirectiveWithNoKind() ||
IsInSingleLineDirective() ||
Expand Down Expand Up @@ -367,18 +377,31 @@ owner.Parent is CSharpCodeBlockSyntax &&
return false;
}

bool IsInHtmlAttributeName()
bool IsInBoundComponentAttributeName()
Copy link
Member Author

@davidwengier davidwengier Feb 15, 2023

Choose a reason for hiding this comment

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

This method name was doing a poor job of describing what it did. Also expanded the comment below to be clearer.

{
// E.g, (| is position)
//
// `<p |csharpattr="Variable">` - true
//
// Because we map attributes, so rename and FAR works, there could be C# mapping for them,
// but only if they're actually bound attributes
// but only if they're actually bound attributes. We don't want the mapping to throw make the
// formatting engine think it needs to apply C# indentation rules.
//
// The exception here is if we're being asked whether to format the line of code at all,
// then we want to pretend it's not a component attribute, because we do still want the line
// formatted. ie, given this:
//
// `<p
// |csharpattr="Variable">`
//
// We want to return false when being asked to format the line, so the line gets indented, but
// return true if we're just being asked "should we format this according to C# rules".

return owner.AncestorsAndSelf().Any(
n => n is MarkupTagHelperAttributeSyntax { TagHelperAttributeInfo: { Bound: true } } or
MarkupTagHelperDirectiveAttributeSyntax { TagHelperAttributeInfo: { Bound: true } });
return owner is MarkupTextLiteralSyntax
{
Parent: MarkupTagHelperAttributeSyntax { TagHelperAttributeInfo: { Bound: true } } or
MarkupTagHelperDirectiveAttributeSyntax { TagHelperAttributeInfo: { Bound: true } }
} && !isLineRequest;
}

bool IsInHtmlAttributeValue()
Expand Down
9 changes: 8 additions & 1 deletion src/Razor/src/Microsoft.AspNetCore.Razor.LanguageServer/Formatting/FormattingVisitor.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -341,7 +341,14 @@ public override void VisitMarkupAttributeBlock(MarkupAttributeBlockSyntax node)

public override void VisitMarkupTagHelperAttribute(MarkupTagHelperAttributeSyntax node)
{
Visit(node.Value);
WriteBlock(node, FormattingBlockKind.Tag, n =>
Copy link
Member Author

@davidwengier davidwengier Feb 15, 2023

Choose a reason for hiding this comment

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

This updates tag helper attributes to match the logic of html tag attributes, in the method above.

I don't fully understand:

  • What this is doing
  • Why it wasn't doing it before
  • Why only one of the tests was fixed with this change
  • Why none of the existing tests broke with this change

Formatting! :)

{
var equalsSyntax = SyntaxFactory.MarkupTextLiteral(new SyntaxList<SyntaxToken>(node.EqualsToken));
var mergedAttributePrefix = SyntaxUtilities.MergeTextLiterals(node.NamePrefix, node.Name, node.NameSuffix, equalsSyntax, node.ValuePrefix);
Visit(mergedAttributePrefix);
Visit(node.Value);
Visit(node.ValueSuffix);
});
}

public override void VisitMarkupTagHelperDirectiveAttribute(MarkupTagHelperDirectiveAttributeSyntax node)
Expand Down
30 changes: 28 additions & 2 deletions ...azor/test/Microsoft.AspNetCore.Razor.LanguageServer.Test/Formatting/HtmlFormattingTest.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -749,7 +749,7 @@ await RunFormattingTestAsync(
tagHelpers: GetComponents());
}

[Fact(Skip = "Requires fix")]
[Fact]
[WorkItem("https://github.com/dotnet/razor/issues/8227")]
public async Task FormatNestedComponents3()
{
Expand All @@ -767,6 +767,19 @@ await RunFormattingTestAsync(
</Frag>
</Component1>
}

@if (true)
{
<a_really_long_tag_name Id="comp1"
Copy link
Member Author

@davidwengier davidwengier Feb 15, 2023

Choose a reason for hiding this comment

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

Added this because html tags formatted correctly, and component tags didn't, so it was convenient when debugging to see what was happening that was different. Figured I'd leave it 🤷‍♂️

Caption="Title" />
<a_really_long_tag_name Id="comp2"
Caption="Title">
<a_really_long_tag_name>
<a_really_long_tag_name Id="comp3"
Caption="Title" />
</a_really_long_tag_name>
</a_really_long_tag_name>
}
""",
expected: """
@if (true)
Expand All @@ -781,6 +794,19 @@ await RunFormattingTestAsync(
</Frag>
</Component1>
}

@if (true)
{
<a_really_long_tag_name Id="comp1"
Caption="Title" />
<a_really_long_tag_name Id="comp2"
Caption="Title">
<a_really_long_tag_name>
<a_really_long_tag_name Id="comp3"
Caption="Title" />
</a_really_long_tag_name>
</a_really_long_tag_name>
}
""",
tagHelpers: GetComponents());
}
Expand Down Expand Up @@ -809,7 +835,7 @@ await RunFormattingTestAsync(
tagHelpers: GetComponents());
}

[Fact(Skip = "Requires fix")]
[Fact]
[WorkItem("https://github.com/dotnet/razor/issues/8229")]
public async Task FormatNestedComponents5()
{
Expand Down