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

Fix Razor formatting bug #6236

Merged

Conversation

allisonchou
Copy link
Contributor

@allisonchou allisonchou commented Aug 26, 2023

Fixes microsoft/vscode-dotnettools#218

Short PR, but a hard one to investigate. The reason why nothing was happening was because the HTML formatter wasn't respecting the html.format.preserveNewLines setting, which in turn made some of our code go from:

@model Test

Text

to

@model Test Text

Since the above is invalid Razor syntax, it caused Razor's FormattingDiagnosticValidationPass to fail, resulting in our formatter doing nothing.

While the HTML formatter failed to respect the html.format.preserveNewLines setting, the HTML range formatter respected it. I was confused as to why this was the case, and started digging into the HTML code. I eventually filed microsoft/vscode#191395 which I believe is the root cause. (Tl;dr HTML isn't explicitly registering a document formatter, which I believe they need to do in order to pick up settings.) Once the HTML-side bug is fixed, Razor should hypothetically be able to remove the workaround in this PR.

Razor formatting will still be broken in the case where the user turns html.format.preserveNewLines off (it's on by default, so hopefully this case is rare), since vscode's FormattingOptions doesn't take custom formatter settings. The real long-term fix here is probably to complete dotnet/razor#7564 on Razor's end, but that seems to be a ways off.

Copy link
Contributor

@davidwengier davidwengier left a comment

Choose a reason for hiding this comment

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

Wow, nice find!

One question, and maybe you don't know the answer which is cool, but if HTML isn't registering a document formatter, then why would it be called at all when we executed the formatting command? Essentially, I wonder if the response will be that this is by design, and the workaround will therefore be permanent. Fine by me if it is, just thinking out loud :)

@allisonchou
Copy link
Contributor Author

Wow, nice find!

One question, and maybe you don't know the answer which is cool, but if HTML isn't registering a document formatter, then why would it be called at all when we executed the formatting command? Essentially, I wonder if the response will be that this is by design, and the workaround will therefore be permanent. Fine by me if it is, just thinking out loud :)

Good question, to be honest I'm not quite sure. My theory is that there's some fallback way they call the formatter, but I couldn't find the exact path through my digging. Will be curious to see how the VS Code HTML team responds. 😊

@allisonchou allisonchou merged commit 0963f18 into dotnet:main Aug 28, 2023
6 checks passed
@allisonchou allisonchou deleted the dev/allichou/FixRazorFormattingBug branch August 28, 2023 19:42
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.

[BUG] Razor Formatting no longer works?
2 participants