Skip to content

Only update snippet model on explicit selection#51948

Merged
sharwell merged 6 commits intodotnet:features/argument-completionfrom
sharwell:explicit-switch
Mar 18, 2021
Merged

Only update snippet model on explicit selection#51948
sharwell merged 6 commits intodotnet:features/argument-completionfrom
sharwell:explicit-switch

Conversation

@sharwell
Copy link
Contributor

@sharwell sharwell commented Mar 18, 2021

  • Update snippet model on explicit selection only (breaks cycles that could occur)
  • Fix FormatSpan interfering with SignatureHelp
  • Add tests for previously-broken behaviors

@sharwell sharwell requested a review from a team as a code owner March 18, 2021 01:22
@ghost ghost added the Area-IDE label Mar 18, 2021
Copy link
Member

@jasonmalinowski jasonmalinowski left a comment

Choose a reason for hiding this comment

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

That was many fewer lines of code than I expected it to be. :shipit:

}

[WpfFact]
public void FullCycle()
Copy link
Contributor Author

@sharwell sharwell Mar 18, 2021

Choose a reason for hiding this comment

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

📝 This test covers a fairly obvious behavior, but one known case where it would fail is if the logic in FormatSpan breaks again

VisualStudio.Editor.Verify.CurrentLineText("Test($$)", assertCaretPosition: true);

VisualStudio.Editor.SendKeys(VirtualKey.Down);
VisualStudio.Editor.Verify.CurrentLineText("Test(0$$)", assertCaretPosition: true);
Copy link
Contributor Author

@sharwell sharwell Mar 18, 2021

Choose a reason for hiding this comment

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

📝 If FormatSpan is running on argument completion snippets, this line would fail because the test would have transitioned to:

-Test(0$$)
+Test()0$$

@sharwell sharwell merged commit b28fcc6 into dotnet:features/argument-completion Mar 18, 2021
@sharwell sharwell deleted the explicit-switch branch March 18, 2021 17:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants