Explicitly pass formatting options to Roslyn APIs (via External Access) #6166
Conversation
|
@davidwengier Does this look reasonable? |
|
@allisonchou who did the initial work on formatting options here. The approach seems fine in general, but perhaps one thing that seems a little inconsistent. On the roslyn side the document is used to get the formatting options, and the syntax root. On the razor side we construct the document but no longer set any formatting options, so it almost seems like maybe we don't need the document at all? Would it be better or worse to just have the razor side pass the syntax root and formatting options only, and remove all of the adhoc workspace and document creation stuff? Unless its used by something else I'm not remembering of course, but we do already have other places in razor where we parse the generated text to get a syntax tree. |
The document is needed to get formatting options from editorconfig |
|
That works?! To be clear, I mean the document created in |
|
I didn't know there is no file path. Is that true for all the documents from the formatting context? Shouldn't they have file path from DocumentUri sent by the LSP client? |
|
At the moment yes. Razor basically doesn't support editorconfig yet (tracked by #4406) and presumably we need to design what it means (do we honour *.cs settings for code analysis, or *.razor? Presumably *.razor for indentation etc. and we'll need to pass that through to C# for generated code etc.) |
|
OK, so if we don't need to support editorconfig then I can simplify this a bit. |
tmat
requested review from
a team,
NTaylorMullen,
ryanbrandenburg,
allisonchou,
sharwell and
davidwengier
as code owners
|
@davidwengier Should be good to merge now, provided that tests pass. |
@tmat they did not. Did you mean to bump the Roslyn version or something? The Razor EA is throwing |
|
@tmat / @davidwengier: Does this PR need to be resurrected or closed? |
# Conflicts: # src/Razor/src/Microsoft.AspNetCore.Razor.LanguageServer/Formatting/CSharpFormatter.cs # src/Razor/test/Microsoft.AspNetCore.Razor.LanguageServer.Test/Formatting/FormattingLanguageServerClient.cs # src/Razor/test/Microsoft.AspNetCore.Razor.LanguageServer.Test/Formatting/FormattingTestBase.cs
|
I've merged in latest and fixed the conflicts. I think it still makes sense to take this, for future usage, but I think it needs updating, for sure. @tmat in the last commit you added a parameter to |
|
As per @davidwengier 's last comment it seems more work is needed to update this PR and we have some file conflicts. @tmat given that this hasn't been updated for a while, would it make sense to close this PR and reopen once it's updated? |
|
Yes, more work is needed but why not keep the PR open in draft mode? |
# Conflicts: # src/Razor/src/Microsoft.AspNetCore.Razor.LanguageServer/Formatting/CSharpFormatter.cs # src/Razor/src/Microsoft.AspNetCore.Razor.LanguageServer/Formatting/CSharpOnTypeFormattingPass.cs # src/Razor/test/Microsoft.AspNetCore.Razor.LanguageServer.Test/Formatting/FormattingLanguageServerClient.cs # src/Razor/test/Microsoft.AspNetCore.Razor.LanguageServer.Test/Formatting/TestRazorFormattingService.cs
There was a problem hiding this comment.
Updated again with latest main.
This didn't actually fix the bug, because of a call to TryApplyChanges in FormattingContext so I removed that as well. The only difference between the current state of this PR and its original one, is the autoFormattingOptions were originally removed in order to use the C# settings but a) its not clear to me if that makes sense and b) it didn't work because it needed to be in Roslyns MEF composition.
This fixes the bug though, so at least moves us forward even if there is more work to do in future.
There was a problem hiding this comment.
Updated again with latest main.
This didn't actually fix the bug, because of a call to TryApplyChanges in FormattingContext so I removed that as well. The only difference between the current state of this PR and its original one, is the autoFormattingOptions were originally removed in order to use the C# settings but a) its not clear to me if that makes sense and b) it didn't work because it needed to be in Roslyns MEF composition.
This fixes the bug though, so at least moves us forward even if there is more work to do in future.
In dotnet/razor#6166 we changed Razor to pass indentation options into a Roslyn service, rather than create a workspace and apply changes to it, etc. Sadly this regressed C# formatting in Razor files, because the code was just creating a new `CSharpSyntaxFormattingOptions` and not starting from the users settings. Fixes https://devdiv.visualstudio.com/DevDiv/_workitems/edit/2305075 Fixes https://devdiv.visualstudio.com/DevDiv/_workitems/edit/2305404
Requires:
FIxes #8638