Skip to content

Conversation

dibarbet
Copy link
Member

@dibarbet dibarbet commented Sep 6, 2025

Tell the client not to modify the whitespace we provided in auto insert snippets.
Requires dotnet/roslyn#80075

@dibarbet dibarbet changed the title Do now allow client to modify whitespace for auto insert edits Do not allow client to modify whitespace for auto insert edits Sep 8, 2025
@dibarbet dibarbet force-pushed the fix_on_auto_insert_whitespace branch from ffb9364 to c5ab09c Compare September 9, 2025 18:42
@dibarbet dibarbet force-pushed the fix_on_auto_insert_whitespace branch from c854d76 to 11bb070 Compare September 9, 2025 18:47
@dibarbet dibarbet marked this pull request as ready for review September 9, 2025 19:56
@dibarbet dibarbet requested a review from a team as a code owner September 9, 2025 19:56
// and another to move the close brace to a new line below that). We still want to trigger on auto insert here, so remove
// duplicates before we check if the change matches a trigger character.
if (changeTrimmed.length > 1) {
changeTrimmed = [...new Set(changeTrimmed)].join('');
Copy link
Member

Choose a reason for hiding this comment

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

changeTrimmed is a string. What is this intending to do?

Copy link
Member Author

Choose a reason for hiding this comment

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

The idea is to remove duplicate newline characters from the string. Potentially this should instead just check if it the string is only new line characters, and remove duplicates then.

Comment on lines +113 to +114
//const editor = vscode.window.activeTextEditor?.insertSnippet();

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
//const editor = vscode.window.activeTextEditor?.insertSnippet();

Copy link
Member

@JoeRobich JoeRobich left a comment

Choose a reason for hiding this comment

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

Can we add a test to the onAutoInsert integration tests?

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