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

Workaround Roslyn formatting bug #2051

Closed
wants to merge 2 commits into from

Conversation

333fred
Copy link
Contributor

@333fred 333fred commented Dec 25, 2020

Due to dotnet/roslyn#50129, FormatAsync will give us changes past the location that we wanted if there were changes to be made on the current line. This can lead to a frustrating typing experience now that we listen for \n in vscode to trigger formatting. I've implemented a simple workaround for this while the bug is fixed upstream.

Due to dotnet/roslyn#50129, `FormatAsync` will give us changes past the location that we wanted if there were changes to be made on the current line. This can lead to a frustrating typing experience now that we listen for `\n` in vscode to trigger formatting. I've implemented a simple workaround for this while the bug is fixed upstream.
…e newlines added are of a different flavor than the ones in the new text, add additional skipped test for incomplete declarations.
}");
}

[Theory(Skip = "https://github.com/dotnet/roslyn/issues/50130")]
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'm not sure there's a good way to work around this one until the upstream bug is fixed.

@333fred
Copy link
Contributor Author

333fred commented Dec 25, 2020

@JoeRobich @filipw so in talking with Cyrus, he pointed out a few things:

  1. Roslyn doesn't even support this type of after-character formatting. Indeed, the editor features on-type formatting supports ;{}#nte:): http://sourceroslyn.io/#Microsoft.CodeAnalysis.CSharp.EditorFeatures/Formatting/CSharpEditorFormattingService.cs,38.
  2. Further, we (roslyn) could consider moving IEditorFormattingService down to the features layer and O# can depend on that, rather than calling format ourselves and likely not passing in the correct options (for paste formatting or return formatting or whatever else).

Given those points, should we even be attempting to format on newlines at all (other than the doc comment feature)? Seems like we're trying to use a roslyn feature in a way it wasn't really intended to be used, and hitting issues because of it.

@filipw
Copy link
Member

filipw commented Dec 25, 2020

thanks for the background info, I now understand the problem better. I like your suggestion to move IEditorFormattingService to features layer, even if it wasn't public we could easily start using it which would be the most optimal solution

@333fred
Copy link
Contributor Author

333fred commented Dec 26, 2020

And did you have an opinion on the format-on-newline bit?

@filipw
Copy link
Member

filipw commented Dec 26, 2020

yes I would say for me it is acceptable for OmniSharp to only support this in targeted scenarios - like doc comments - and not have this everywhere.

@333fred 333fred closed this Dec 27, 2020
@333fred 333fred deleted the workaround-50129 branch December 27, 2020 03:20
@333fred
Copy link
Contributor Author

333fred commented Dec 27, 2020

Submitted #2053 to remove the support entirely.

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.

2 participants