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

Jump to bottom

LSP Razor formatting for Razor code block directives #1573

Merged
merged 4 commits into from
Feb 14, 2020
Merged

LSP Razor formatting for Razor code block directives #1573

merged 4 commits into from
Feb 14, 2020

Conversation

ajaybhargavb
Copy link
Contributor

@ajaybhargavb ajaybhargavb commented Feb 12, 2020 •
edited
Loading

https://github.com/dotnet/aspnetcore/issues/14271

I apologize for the large PR but a lot of it is plumbing, extension methods and tests. The bulk of the logic is contained within DefaultRazorFormattingService. I added a fair amount of abstraction but I decided to not abstract away too many things just yet because I expect more changes to the design when we support HTML formatting as well.

  • Support for @code/@functions block formatting
    • Except when it contains Markup or other Razor constructs
  • Added a RazorFormattingService which is invoked by the RazorFormattingEndpoint.
  • Added a custom razor/rangeFormatting command that the server can use to ask the client to format a range of the projected C# or HTML document
  • Added a CSharpFormatter and HTMLFormatter that invoke the above mentioned command
  • Added FormattingSpan and its corresponding visitor to represent Razor understanding of indentation
  • Moved the document mapping code to a separate RazorDocumentMappingService service for ease of use
  • Added necessary extension methods for convenience. Some of them were copied from Roslyn
  • Some cleanup
  • Added a C# test formatter to enable unit testing. Right now it calls Roslyn APIs directly. As far as I've seen its behavior is the same as OmniSharp formatting except it doesn't remove trailing whitespace and empty lines. I am following up with people to understand why that is the case.
  • Added/updated tests

NTaylorMullen
NTaylorMullen suggested changes Feb 12, 2020
View reviewed changes
Copy link
Contributor

@NTaylorMullen NTaylorMullen left a comment

Choose a reason for hiding this comment

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

Looking really good. I love the separation you made with a lot of the concepts and the testing strategy you landed on is a perfect middleground.

👏 🎉 🥂

src/Razor/src/Microsoft.AspNetCore.Razor.LanguageServer/Formatting/CSharpFormatter.cs Outdated Show resolved Hide resolved
src/Razor/src/Microsoft.AspNetCore.Razor.LanguageServer/DefaultRazorDocumentMappingService.cs Show resolved Hide resolved
src/Razor/src/Microsoft.AspNetCore.Razor.LanguageServer/Formatting/CSharpFormatter.cs Show resolved Hide resolved
src/Razor/src/Microsoft.AspNetCore.Razor.LanguageServer/Formatting/FormattingContext.cs Outdated Show resolved Hide resolved
src/Razor/src/Microsoft.AspNetCore.Razor.LanguageServer/Formatting/FormattingVisitor.cs Show resolved Hide resolved
...or/src/Microsoft.AspNetCore.Razor.LanguageServer/Formatting/DefaultRazorFormattingService.cs Outdated Show resolved Hide resolved
.../Microsoft.AspNetCore.Razor.LanguageServer.Test/Formatting/FormattingLanguageServerClient.cs Show resolved Hide resolved
...crosoft.AspNetCore.Razor.LanguageServer.Test/Formatting/DefaultRazorFormattingServiceTest.cs Show resolved Hide resolved
...crosoft.AspNetCore.Razor.LanguageServer.Test/Formatting/DefaultRazorFormattingServiceTest.cs Show resolved Hide resolved
...crosoft.AspNetCore.Razor.LanguageServer.Test/Formatting/DefaultRazorFormattingServiceTest.cs Show resolved Hide resolved
@dotnet dotnet deleted a comment from NTaylorMullen Feb 13, 2020
@ajaybhargavb
Copy link
Contributor Author

🆙 📅

NTaylorMullen
NTaylorMullen approved these changes Feb 13, 2020
View reviewed changes
Copy link
Contributor

@NTaylorMullen NTaylorMullen left a comment

Choose a reason for hiding this comment

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

Looks good. Before checking in could you expand this comment to ensure future viewers/readers fully understand the problem.

...or/src/Microsoft.AspNetCore.Razor.LanguageServer/Formatting/DefaultRazorFormattingService.cs Show resolved Hide resolved
ryanbrandenburg
ryanbrandenburg approved these changes Feb 13, 2020
View reviewed changes
Copy link
Contributor

@ryanbrandenburg ryanbrandenburg left a comment

Choose a reason for hiding this comment

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

Some questions but nothing groundbreaking.

src/Razor/src/Microsoft.AspNetCore.Razor.LanguageServer/DefaultRazorDocumentMappingService.cs Show resolved Hide resolved
src/Razor/src/Microsoft.AspNetCore.Razor.LanguageServer/Formatting/HtmlFormatter.cs Outdated Show resolved Hide resolved
src/Razor/src/Microsoft.AspNetCore.Razor.LanguageServer/Formatting/HtmlFormatter.cs Show resolved Hide resolved
src/Razor/src/Microsoft.AspNetCore.Razor.LanguageServer/Formatting/IndentationContext.cs Show resolved Hide resolved
src/Razor/src/Microsoft.AspNetCore.Razor.LanguageServer/Formatting/RazorSyntaxTreeExtensions.cs Show resolved Hide resolved
src/Razor/src/Microsoft.AspNetCore.Razor.LanguageServer/RazorLanguageEndpoint.cs Show resolved Hide resolved
src/Razor/src/Microsoft.AspNetCore.Razor.LanguageServer/RazorLanguageEndpoint.cs Outdated Show resolved Hide resolved
src/Razor/src/Microsoft.AspNetCore.Razor.LanguageServer/SourceTextExtensions.cs Show resolved Hide resolved
@ajaybhargavb ajaybhargavb merged commit 62051b9 into master Feb 14, 2020
@ajaybhargavb ajaybhargavb deleted the ajbaaska/better-formatting branch February 14, 2020 00:37
@ajaybhargavb ajaybhargavb mentioned this pull request Feb 14, 2020
Closed
JunTaoLuo pushed a commit to dotnet/aspnetcore that referenced this pull request May 17, 2020
@ajaybhargavb
LSP Razor formatting for Razor code block directives (dotnet/razor#1573)
f1ccf85 
LSP Razor formatting for Razor code block directives
- Support for @code/@functions block formatting
    - Except when it contains Markup or other Razor constructs
- Added a RazorFormattingService which is invoked by the RazorFormattingEndpoint.
- Added a custom razor/rangeFormatting command that the server can use to ask the client to format a range of the projected C# or HTML document
- Added a CSharpFormatter and HTMLFormatter that invoke the above mentioned command
- Added FormattingSpan and its corresponding visitor to represent Razor understanding of indentation
- Moved the document mapping code to a separate RazorDocumentMappingService service for ease of use
- Added necessary extension methods for convenience. Some of them were copied from Roslyn
- Some cleanup
- Added a C# test formatter to enable unit testing. Right now it calls Roslyn APIs directly. As far as I've seen its behavior is the same as OmniSharp formatting except it doesn't remove trailing whitespace and empty lines. I am following up with people to understand why that is the case.
Added/updated tests
\n\nCommit migrated from dotnet/razor@62051b9
dougbu pushed a commit to dougbu/razor-compiler that referenced this pull request Nov 17, 2021
@ajaybhargavb
LSP Razor formatting for Razor code block directives (dotnet/razor#1573)
9204abf 
LSP Razor formatting for Razor code block directives
- Support for @code/@functions block formatting
    - Except when it contains Markup or other Razor constructs
- Added a RazorFormattingService which is invoked by the RazorFormattingEndpoint.
- Added a custom razor/rangeFormatting command that the server can use to ask the client to format a range of the projected C# or HTML document
- Added a CSharpFormatter and HTMLFormatter that invoke the above mentioned command
- Added FormattingSpan and its corresponding visitor to represent Razor understanding of indentation
- Moved the document mapping code to a separate RazorDocumentMappingService service for ease of use
- Added necessary extension methods for convenience. Some of them were copied from Roslyn
- Some cleanup
- Added a C# test formatter to enable unit testing. Right now it calls Roslyn APIs directly. As far as I've seen its behavior is the same as OmniSharp formatting except it doesn't remove trailing whitespace and empty lines. I am following up with people to understand why that is the case.
Added/updated tests
\n\nCommit migrated from dotnet/razor@62051b9

Commit migrated from dotnet/aspnetcore@f1ccf855ebfc
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@NTaylorMullen NTaylorMullen NTaylorMullen approved these changes

@ryanbrandenburg ryanbrandenburg ryanbrandenburg approved these changes

@rynowak rynowak Awaiting requested review from rynowak

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@ajaybhargavb @NTaylorMullen @ryanbrandenburg