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

Call Roslyn to format new code behind documents #9263

Merged

Conversation

davidwengier
Copy link
Contributor

@davidwengier davidwengier commented Sep 11, 2023

Fixes #4330
Fixes #8766
Includes #9262 so just review from 8a2b8af onwards

Goes with dotnet/roslyn#69878 and dotnet/vscode-csharp#6329

I logged #9264 to follow up with a better test, though strictly speaking the one I've added id exhaustive :)

Behaviour of Extract to Code Behind before this change. Note the many using statements and block scoped namespace.
ExtractToCodeBehindBefore

Behaviour after this change. Note the file scoped namespace, and file header.
ExtractToCodeBehindAfter

The squiggle on NavigationManager is due to an .editorconfig rule I have on in that project, requiring this. qualification, so is unrelated.

The node used to create the ExtractStart property actually starts at the end of the directive, on the line before the opening brace.
This is a silly test, but strictly speaking its correct, as our logic is to take whatever Roslyn gives us. There are tests on the Roslyn side to validate that they format the document correctly etc.
Comment on lines 181 to 182
.GetDocumentIntermediateNode()
.FindDescendantNodes<UsingDirectiveIntermediateNode>();
Copy link
Member

Choose a reason for hiding this comment

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

nit: indentation

Comment on lines +225 to +226
// Sadly we can't use a "real" workspace here, because we don't have access. If we use our workspace, it wouldn't have the right settings
// for C# formatting, only Razor formatting, and we have no access to Roslyn's real workspace, since it could be in another process.
Copy link
Member

Choose a reason for hiding this comment

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

Oh, this is weird. Thanks for adding the comment to explain. However, if we just pass an instance of AdhocWorkspace, I imagine that it's only ever going to get a default set of C# formatting options and will ignore anything the user might have set in VS Options or .editorconfig. Do you know what we need trom Roslyn to call into formatting without a workspace? I seem to remember that @CyrusNajmabadi was working on some sort of mechanism (SolutionServices?).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Firstly, worth mentioning this path is now just a fallback until the Roslyn changes are inserted. Once that is in I wouldn't expect the above call to return null.

Do you know what we need trom Roslyn to call into formatting without a workspace?

The Format method takes a SyntaxFormattingOptions, which you can see in the "real" call here: https://github.com/dotnet/roslyn/pull/69878/files#diff-7ab7cb7fc8b5d3ba31e8bbbdcd96a9dfcadc129bde4258aa73a91ab4b4720bfbR82
In theory we could call Roslyn to get that info, but if we're going to call Roslyn then the call on line 221 would have worked and we don't need this anyway :)

@davidwengier davidwengier merged commit da56c44 into dotnet:main Sep 14, 2023
12 checks passed
@davidwengier davidwengier deleted the CallRoslynForExtractToCodeBehind branch September 14, 2023 07:46
@ghost ghost added this to the Next milestone Sep 14, 2023
@Cosifne Cosifne modified the milestones: Next, 17.8 P3 Sep 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants