Moving formatting service to common layer#10761
Conversation
|
|
It requires services not easily available and will require extra work. For now leaving it in LSP server layer. Current need for the formatting service in cohosting is for OnAutoInsert which doesn't need HTML formatting.
18d9e8e to
0291c12
Compare
DustinCampbell
left a comment
There was a problem hiding this comment.
Other than some minor clean up feedback (that you can take or leave), looks good to me!
| return false; | ||
|
|
||
| static bool TryGetWhitespace(SyntaxList<RazorSyntaxNode> children, [NotNullWhen(true)] out CSharpStatementLiteralSyntax? whitespaceBeforeSectionName, [NotNullWhen(true)] out UnclassifiedTextLiteralSyntax? whitespaceAfterSectionName) | ||
| static bool TryGetWhitespace(AspNetCore.Razor.Language.Syntax.SyntaxList<RazorSyntaxNode> children, [NotNullWhen(true)] out CSharpStatementLiteralSyntax? whitespaceBeforeSectionName, [NotNullWhen(true)] out UnclassifiedTextLiteralSyntax? whitespaceAfterSectionName) |
There was a problem hiding this comment.
Consider adding a using type alias at the type of the file for SyntaxList:
using RazorSyntaxNodeList = Microsoft.AspNetCore.Razor.Language.Syntax.SyntaxList<Microsoft.AspNetCore.Razor.Language.Syntax.RazorSyntaxNode>;There was a problem hiding this comment.
@davidwengier: It feels like some global usings for common type aliases like these might be useful. What do you think?
There was a problem hiding this comment.
@davidwengier: It feels like some global usings for common type aliases like these might be useful. What do you think?
I personally would love that for consistency, totally open to suggestions here.
There was a problem hiding this comment.
Also since there is a list of SyntaxNode and a list of RazorSyntaxNode, I ended up with RazorSyntaxNodeList and RazorRazorSyntaxNodeList :) The latter sound silly though follows the pattern. Suggestions are welcome.
There was a problem hiding this comment.
I honestly do not understand why Razor has both SyntaxNode and RazorSyntaxNode. It's very unclear and inconsistent. RazorSyntaxNode inherits directly from SyntaxNode and adds no extra implementation. Most SyntaxNode subtypes in the Razor compiler actually inherit from RazorSyntaxNode. Heck, even CSharpSyntaxNode inherits from RazorSyntaxNode. There are only a few types that directly inherit from SyntaxNode, and I wonder if those are mistakes? For example, SyntaxToken inherits from RazorSyntaxNode but SyntaxTrivia inherits from SyntaxNode? It's all wacky. 😄
There was a problem hiding this comment.
I certainly don't know of any nuances between these. While I suspect there may have once been an attempt to separate razor from C#/VB, as you mentioned it doesn't exist today. The whole syntax implementation is a bit wacky, given the whole "copied from pre-CTP 1 Roslyn" nature of it.
There was a problem hiding this comment.
Yeah, I just chatted with @chsienki and he didn't know of any reason. It doesn't look to me like anything actually creates SyntaxTrivia instances. That one could probably just be removed.
There was a problem hiding this comment.
We're thinking about using SyntaxTrivia if the request for razor preprocessor directives ever goes forward.
There was a problem hiding this comment.
@davidwengier: It feels like some global usings for common type aliases like these might be useful. What do you think?
No complaints. These type names have always been annoying. Sometimes SyntaxNode is a razor node, sometimes its a roslyn node 🤷♂️
* upstream/main: (71 commits) Fix after merge PR feedback Bump Roslyn version Moving formatting service to common layer (dotnet#10761) Move GetSyntaxTree to document snapshot Inject file path service into the document snapshot Remove code document parameter and just use document snapshot Update NOTICE.txt (dotnet#10768) Allow @@ as a fallback (dotnet#10752) Rework how we get generated documents Directly test the component definition service in cohosting Add missing test case Defer to C# for component attribute GTD in cohosting Allow LSP and cohosting to provide specialized methods to get a syntax tree for a C# document Dev Container (dotnet#10751) Use a proper Try pattern Add tests for co-hosted GTD Rework IDocumentPositionInfoStrategy and use correctly in co-hosted GTD Add DocumentMappingSerice to RazorDocumentServiceBase Move IDocumentPositionInfoStrategy and friends to Workspaces layer ...
| // Disclaimer: CSharpCodeBlockSyntax is used a _lot_ in razor so these methods are probably | ||
| // being overly careful to only try to format syntax forms they care about. | ||
| TryFormatCSharpBlockStructure(context, edits, source, node); | ||
| TryFormatCSharpBlockStructure(context, edits, source, node); // TODO |
There was a problem hiding this comment.
My bad, that should've been deleted. It was added when I was passing in hardcoded value for "bracesOnNextLine" options. I ended up not passing it in the parameter list for this method, but forgot to remove TODO. Happy to remove it unless you are editing that file as a part of PR, in which case I'd appreciate if you removed it.
Summary of the changes
Moving RazorFormattingService to common layer. It's immediately needed for OnAutoInsert support, but will eventually be needed by the formatting endpoints themselves.
Fixes:
Part of #9519