-
Notifications
You must be signed in to change notification settings - Fork 4k
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
Commit C# completion on colon #12852
Conversation
{ | ||
internal class CSharpCompletionItemRules | ||
{ | ||
public static readonly CompletionItemRules Instance = CompletionItemRules.Default |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Comment this.
Thinking more on this, this doesn't seem like the right way to fix this issue. Instead, it should be hte NamedParameterCompletionPRovider taht returns items that have this letter removed from their filter set. Also, have you fixed up VB and tested that as well? |
@CyrusNajmabadi Done. Per our slack conversation I don't think there are any changes to be made to VB. |
@rchande Cool. |
} | ||
} | ||
"; | ||
await VerifyProviderCommitAsync(markup, "args:", expected, ':', "arg"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you actually add a COmpletionHandlerTest test. I don't trust these tests for actually doing commit properly. The completion provider test harness (which you're using here) should really only be used for asking "what completion items should be offered". If you want to test Commit, you really need to go through the CommandHandler test as they're the only ones that exercise the full stack properly.
Add the test and fix the nits, and 👍 |
Agree with both of @CyrusNajmabadi's points above. |
4847db4
to
83d299c
Compare
Tagging @dotnet/roslyn-ide for review