Skip to content

[LSP] Respect settings passed in by LSP in AutoInsertHandler#51916

Merged
allisonchou merged 2 commits intodotnet:mainfrom
allisonchou:LSPOnAutoInsertSettings
Mar 17, 2021
Merged

[LSP] Respect settings passed in by LSP in AutoInsertHandler#51916
allisonchou merged 2 commits intodotnet:mainfrom
allisonchou:LSPOnAutoInsertSettings

Conversation

@allisonchou
Copy link
Contributor

Today, LSP passes in tabs/spaces settings as part of the OnAutoInsert request. However, we do not respect these settings and instead use the document's options. We should respect the LSP settings.

This will partially fix some of the tabs vs. spaces issues seen in Razor.

@allisonchou allisonchou added Area-IDE LSP issues related to the roslyn language server protocol implementation labels Mar 16, 2021
@allisonchou allisonchou requested a review from a team as a code owner March 16, 2021 21:03
var service = document.GetRequiredLanguageService<IDocumentationCommentSnippetService>();

// We should use the options passed in by LSP instead of the document's options.
var options = await document.GetOptionsAsync(cancellationToken).ConfigureAwait(false);
Copy link
Member

Choose a reason for hiding this comment

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

are the options passed in by LSP correct? Like for example if you have an editor config setting for tabs vs spaces. Does LSP read that? Similarly for tools options settings.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

From testing with Razor, it correctly recognizes changes in Tools->Option settings. I'm not sure about editorconfig, will test shortly.

Regardless, I think we need this change since Razor files won't be correctly formatted without it, namely because we need to use Razor's Tools->Options settings instead of C#'s.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It does indeed seem to support editorconfig as well (at least in C#, I think I recall editorconfig currently being unsupported for Razor files).

Copy link
Member

Choose a reason for hiding this comment

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

Ok, I think it's probably fine to make this change, just one more question

Regardless, I think we need this change since Razor files won't be correctly formatted without it, namely because we need to use Razor's Tools->Options settings instead of C#'s.

Ah, and I guess with the dynamic file info they provide they don't have a way to set those options on the generated document?

Choose a reason for hiding this comment

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

Ah, and I guess with the dynamic file info they provide they don't have a way to set those options on the generated document?

Even if that were the case I wouldn't imagine we'd want to. Given the platform is the owner of settings it should be the one informing you on what settings to use. Out of curiosity does on type formatting or document formatting have this same gap?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Out of curiosity does on type formatting or document formatting have this same gap?

From a glance, this does seem to be the case. I can address this in a follow up PR.

@allisonchou allisonchou merged commit 9ee86a3 into dotnet:main Mar 17, 2021
@allisonchou allisonchou deleted the LSPOnAutoInsertSettings branch March 17, 2021 23:43
@ghost ghost added this to the Next milestone Mar 17, 2021
@allisonchou allisonchou modified the milestones: Next, 16.10.P2 Mar 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Area-IDE LSP issues related to the roslyn language server protocol implementation

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants