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.

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

Re-architect formatting to prepare for cohosting (and for fun!) #10778

Merged
merged 28 commits into from
Aug 30, 2024

Conversation

davidwengier
Copy link
Contributor

I nerd sniped myself thinking about how to get formatting into cohosting, given the limitations Alex ran into doing the relayering for auto insert, and this is the result. I was going to go further and port the actual formatting endpoint to cohosting, but that would have ran into the same issue that Alex did with auto insert, so I figured I'd wait for that to merge, and put this up in the meantime.

This unblocks the formatting, code action and completion end points from being ported.
Part of #10743
Part of #9519

I strongly recommend reviewing commit-at-a-time, as I did this deliberately in an order, and in order to (hopefully) make reviewing easier. Though granted, there are a lot of commits.

This commit is just moves, no functionality changes. Minor tweaks to method visibility, and one rename of a static field :)
This probably looks like overkill, but hopefully the next commit will help explain a little about what is going on.
Always felt like a huge potential bug farm. eg, if Html ended up not being first we'd have bugs, working out the Order property (which was weirdly backwards?) was a pain, and the entire formatting engine produces horrible results if the ordering changes anyway.
Doing this separately, and purely mechanically, so make review easier
@davidwengier davidwengier requested a review from a team as a code owner August 22, 2024 06:21
This was only used for validation as edits pass through the pipeline, but since we now tightly control the pipeline its unnecessary
Comment on lines 9 to 13
internal sealed record RazorFormattingOptions
{
public bool InsertSpaces { get; init; }
public int TabSize { get; init; }
public bool CodeBlockBraceOnNextLine { get; init; }
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
internal sealed record RazorFormattingOptions
{
public bool InsertSpaces { get; init; }
public int TabSize { get; init; }
public bool CodeBlockBraceOnNextLine { get; init; }
internal sealed record RazorFormattingOptions(bool InsertSpaces, int TabSize, bool CodeBlockBraceOnNextLine)
{

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I kinda like not having to specify all options, but of course I realised that I stuffed up the default instance, so I've fixed that. I've copied the style from Roslyn (for example), except moved the static to the top since I'm not an animal. Let me know if you feel strongly about constructors vs initializers

Copy link
Member

Choose a reason for hiding this comment

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

At the very least, I would expect InsertSpaces and TabSize to be specified every time. If that's true, then maybe you could just add a default value to the CodeBlockBraceOnNextLine parameter.

All of that said, I'm definitely sympathetic to non-positional records. You lose deconstruction, but you also lose the friction of having to update constructors when you need to add more properties in the future. That's totally a judgement call and I'm happy either way!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That friction was my main motivation for this type existing in the first place. Plumbing through another flag on FormattingContext was what I started having to do, which led to the creation of this type. Also once Alex's PR is in, there is another flag to come here for FUSE. Though I will admit, having the last parameter be optional, and the new FUSE one, would probably be just as good, as they're only needed for full document formatting, so only one place would have to pay the price.

I'll think about it some more. Not merging this until Alex's PR is in anyway.

Copy link
Member

@DustinCampbell DustinCampbell left a comment

Choose a reason for hiding this comment

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

Looks great to me!

Copy link
Contributor

@ryzngard ryzngard left a comment

Choose a reason for hiding this comment

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

:shipit:

# Conflicts:
#	src/Razor/benchmarks/Microsoft.AspNetCore.Razor.Microbenchmarks/LanguageServer/RazorCSharpFormattingBenchmark.cs
#	src/Razor/src/Microsoft.AspNetCore.Razor.LanguageServer/AutoInsert/OnAutoInsertEndpoint.cs
#	src/Razor/src/Microsoft.AspNetCore.Razor.LanguageServer/Formatting/HtmlFormatter.cs
#	src/Razor/src/Microsoft.AspNetCore.Razor.LanguageServer/InlineCompletion/InlineCompletionEndPoint.cs
#	src/Razor/src/Microsoft.CodeAnalysis.Razor.Workspaces/Formatting/FormattingContext.cs
#	src/Razor/src/Microsoft.CodeAnalysis.Razor.Workspaces/Formatting/IRazorFormattingService.cs
#	src/Razor/src/Microsoft.CodeAnalysis.Razor.Workspaces/Formatting/Passes/CSharpFormattingPass.cs
#	src/Razor/src/Microsoft.CodeAnalysis.Razor.Workspaces/Formatting/Passes/FormattingContentValidationPass.cs
#	src/Razor/src/Microsoft.CodeAnalysis.Razor.Workspaces/Formatting/Passes/FormattingDiagnosticValidationPass.cs
#	src/Razor/src/Microsoft.CodeAnalysis.Razor.Workspaces/Formatting/Passes/HtmlFormattingPassBase.cs
#	src/Razor/src/Microsoft.CodeAnalysis.Razor.Workspaces/Formatting/RazorFormattingService.cs
#	src/Razor/src/Microsoft.CodeAnalysis.Remote.Razor/Formatting/RemoteCSharpOnTypeFormattingPass.cs
#	src/Razor/src/Microsoft.CodeAnalysis.Remote.Razor/Formatting/RemoteRazorFormattingPass.cs
#	src/Razor/test/Microsoft.AspNetCore.Razor.LanguageServer.Test/AutoInsert/RazorOnAutoInsertProviderTestBase.cs
#	src/Razor/test/Microsoft.AspNetCore.Razor.LanguageServer.Test/Completion/Delegation/DelegatedCompletionItemResolverTest.NetFx.cs
#	src/Razor/test/Microsoft.AspNetCore.Razor.LanguageServer.Test/Formatting_NetFx/FormattingLanguageServerTestBase.cs
#	src/Razor/test/Microsoft.AspNetCore.Razor.LanguageServer.Test/Formatting_NetFx/FormattingTestBase.cs
#	src/Razor/test/Microsoft.AspNetCore.Razor.LanguageServer.Test/Formatting_NetFx/TestRazorFormattingService.cs
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants