Experimental support HTML formatting in Razor LSP #2445

ajaybhargavb · 2020-08-28T02:31:57Z

Part of https://github.com/dotnet/aspnetcore/issues/23530, https://github.com/dotnet/aspnetcore/issues/14271

Summary of the changes

This introduces v1 of HTML and C# mixed formatting support

Added support for C# to be formatted on the server
Added HtmlFormattingPass
Added CSharpFormattingPass
Removed legacy C# formatting code
Cleanup

Known issues:

Mixed blocks don't format correctly in some cases
Implicit statements don't format correctly (@if (...){...})
C# specific settings on the client will not be respected
Other rough edges

In the next PR:

Structure validator based on syntax tree structure
Tests

Note: Feel free to try out my changes in your experimental instance to look for obvious corner cases I may have missed.

ajaybhargavb · 2020-08-28T02:35:23Z

src/Razor/src/Microsoft.AspNetCore.Razor.LanguageServer/Formatting/FormattingPassBase.cs

+            return changedText;
+        }
+
+        private static bool ShouldCleanup(FormattingContext context, Position position)


This method is looks pretty similar to ShouldFormat method in the CSharpFormattingPass but it is slightly different. Leaving it separate for now but will merge them in the future if possible.

Is it just the implicit statement difference?

For now, yes.

src/Razor/src/Microsoft.AspNetCore.Razor.LanguageServer/Formatting/CSharpFormatter.cs

src/Razor/src/Microsoft.AspNetCore.Razor.LanguageServer/Formatting/CSharpFormattingPass.cs

NTaylorMullen · 2020-08-28T16:47:53Z

src/Razor/src/Microsoft.AspNetCore.Razor.LanguageServer/Formatting/FormattingPassBase.cs

+            return changedText;
+        }
+
+        private static bool ShouldCleanup(FormattingContext context, Position position)


Is it just the implicit statement difference?

src/Razor/src/Microsoft.AspNetCore.Razor.LanguageServer/Formatting/HtmlFormattingPass.cs

src/Razor/src/Microsoft.AspNetCore.Razor.LanguageServer/Formatting/CSharpFormattingPass.cs

TanayParikh · 2020-08-28T22:44:26Z

src/Razor/src/Microsoft.AspNetCore.Razor.LanguageServer/Formatting/CSharpFormatter.cs

+            CancellationToken cancellationToken)
+        {
+            var workspace = _projectSnapshotManagerAccessor.Instance.Workspace;
+            var cSharpOptions = workspace.Options


Are we enforcing csharp vs cSharp?

Good question. This may have been a copy-paste from somewhere. I like csharp because cSharp kind of takes longer to type 😆. But I am willing to stick to whatever is most used.

csharp +1

+1 on csharp. Looks cleaner in my opinion.

@if

Known issues: - Mixed blocks don't format correctly in some cases - Implicit statements don't format correctly (@if (...){...}) - Other rough edges

ajaybhargavb

Updated with latest rebase, bug fixes, existing test fixes and diagnostic validator.

Will send a separate PR for HTML tests.

ajaybhargavb · 2020-09-01T00:23:33Z

src/Razor/src/Microsoft.AspNetCore.Razor.LanguageServer/Formatting/FormattingVisitor.cs

@@ -60,9 +60,12 @@ public override void VisitCSharpCodeBlock(CSharpCodeBlockSyntax node)
                node.Parent is CSharpImplicitExpressionBodySyntax ||
                node.Parent is RazorDirectiveBodySyntax ||
                (_currentBlockKind == FormattingBlockKind.Directive &&
-                node.Children.Count == 1 &&
-                node.Children[0] is CSharpStatementLiteralSyntax))
+                node.Parent?.Parent is RazorDirectiveBodySyntax))


Not super proud of this if block here. I didn't want to change this too much because I couldn't recall why some of these existed (I added this 6 months ago without comments. My bad). I've left a comment but I want to clean this up soon.

ghost · 2020-09-01T00:41:18Z

Hello @ajaybhargavb!

Because this pull request has the auto-merge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.

ajaybhargavb requested review from NTaylorMullen, ryanbrandenburg and TanayParikh as code owners August 28, 2020 02:31

ajaybhargavb commented Aug 28, 2020

View reviewed changes

NTaylorMullen approved these changes Aug 28, 2020

View reviewed changes

TanayParikh approved these changes Aug 28, 2020

View reviewed changes

ajaybhargavb added 2 commits August 31, 2020 16:48

Experimental support HTML formatting in Razor LSP

05874d9

Known issues: - Mixed blocks don't format correctly in some cases - Implicit statements don't format correctly (@if (...){...}) - Other rough edges

Bug fixes and fixed tests

40fb27d

ajaybhargavb force-pushed the ajbaaska/html-formatting branch from 6de0dd6 to 40fb27d Compare September 1, 2020 00:21

ajaybhargavb commented Sep 1, 2020

View reviewed changes

NTaylorMullen approved these changes Sep 1, 2020

View reviewed changes

ajaybhargavb added the auto-merge Squash merge once all PR checks are complete and reviewers have approved label Sep 1, 2020

ajaybhargavb removed the auto-merge Squash merge once all PR checks are complete and reviewers have approved label Sep 1, 2020

Fix formatting in VSCode

149954b

ajaybhargavb added the auto-merge Squash merge once all PR checks are complete and reviewers have approved label Sep 1, 2020

TanayParikh approved these changes Sep 1, 2020

View reviewed changes

ghost merged commit 5bfdc63 into master Sep 1, 2020

ghost deleted the ajbaaska/html-formatting branch September 1, 2020 02:51

ajaybhargavb mentioned this pull request Sep 1, 2020

Razor formatting dotnet/vscode-csharp#4030

Closed

NTaylorMullen mentioned this pull request Oct 1, 2021

Formatting #4358

Closed

11 tasks

NTaylorMullen mentioned this pull request Oct 9, 2021

On-by-default Razor formatting #5326

Closed

12 tasks

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Experimental support HTML formatting in Razor LSP #2445

Experimental support HTML formatting in Razor LSP #2445

ajaybhargavb commented Aug 28, 2020 •

edited

Loading

ajaybhargavb Aug 28, 2020

NTaylorMullen Aug 28, 2020

ajaybhargavb Aug 28, 2020

NTaylorMullen Aug 28, 2020

TanayParikh Aug 28, 2020

ajaybhargavb Aug 28, 2020

NTaylorMullen Aug 28, 2020

TanayParikh Aug 28, 2020

ajaybhargavb left a comment •

edited

Loading

ajaybhargavb Sep 1, 2020

ghost commented Sep 1, 2020

Experimental support HTML formatting in Razor LSP #2445

Experimental support HTML formatting in Razor LSP #2445

Conversation

ajaybhargavb commented Aug 28, 2020 • edited Loading

Summary of the changes

Known issues:

In the next PR:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ajaybhargavb left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost commented Sep 1, 2020

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

ajaybhargavb commented Aug 28, 2020 •

edited

Loading

ajaybhargavb left a comment •

edited

Loading

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.