Skip to content

Conversation

dibarbet
Copy link
Member

@dibarbet dibarbet commented Feb 6, 2025

Resolves #7963
Resolves #6244
Resolves microsoft/vscode-dotnettools#1651

Previously, we would completely ignore the values of tabSize/indentSize/insertSpaces when editor.detectIndentation was enabled (which it is by default). Instead, a hardcoded value would be sent to the server.

Normally this isn't super noticeable - features like formatting pass the settings specific to that document (based on the detection) when invoking the language server. However some features like code actions do not specify these options, and so the code would only ever be generated with the hardcoded defaults.

This PR updates it so that we now use the user's settings instead of a hardcoded default even when editor.detectIndentation is enabled. (Note - .editorconfig values still override the user setting as normal).

There is still a caveat here however - we are not using the real detected indentation. So it is still possible that we generate code that does not match the rest of the document's formatting (because the detected indentation does not match the user's setting).
At some point I need to investigate if it is possible to sync the detected per document formatting settings over to the server (so code actions use the detected indentation).

spaces_with_detect_indentation

@dibarbet dibarbet requested a review from a team as a code owner February 6, 2025 01:36
@dibarbet dibarbet requested a review from JoeRobich February 6, 2025 01:36
@JoeRobich
Copy link
Member

JoeRobich commented Feb 6, 2025

Do changes to editor.detectIndentation trigger an option sync? I imagine that formatting options may change when it changes.

edit: haha I reread and thought about it more. Formatting settings are really at a document level. If there is no source of truth like an .editorconfig, then each document could have different detected indentation. Seems like it would be hard to give formatting analyzer what it needs. Although maybe you could sync a specific documents formatting prior to resolving a code action. If the code action being resolved were a fix-all, then we are out of luck.

@dibarbet
Copy link
Member Author

dibarbet commented Feb 6, 2025

Do changes to editor.detectIndentation trigger an option sync? I imagine that formatting options may change when it changes.

We don't sync this option specifically, we do sync the other whitespace settings though. I don't think we need to know about detectIndentation on the server side

Formatting settings are really at a document level. If there is no source of truth like an .editorconfig, then each document could have different detected indentation.

agreed - and there might be a way in LSP to get the per document settings (you can pass resources to the client when you request settings). But we'd have to come up with some mechanism on the server side to store that information per document as well.

An alternative for document-only code actions is that the LSP client adds the options to the request (similar to how the formatting commands work). Of course that wouldn't work for workspace edits either.

@dibarbet dibarbet merged commit cfac77e into dotnet:main Feb 6, 2025
16 checks passed
@dibarbet dibarbet deleted the sync_whitespace branch February 6, 2025 22:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants