Skip to content
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

Don't set completion text to full signature unless its override or partial method completion #868

Merged
merged 2 commits into from
May 19, 2017

Conversation

DustinCampbell
Copy link
Contributor

This change fixes a regression where overloads would not be collapsed in the VS Code completion list. In addition, I added a couple of tweaks to ensure that we report the "EnumMember" and "Const" kinds.

Copy link
Member

@david-driscoll david-driscoll left a comment

Choose a reason for hiding this comment

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

Nit on nameof otherwise looks good.

Should we consider adding a test for this case to avoid this in the future, perhaps as a second PR?

@@ -13,24 +12,36 @@ namespace OmniSharp.Roslyn.CSharp.Services.Intellisense
{
internal static class CompletionItemExtensions
{
private const string GetSymbolsAsync = "GetSymbolsAsync";
Copy link
Member

Choose a reason for hiding this comment

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

Use named?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks!

private const string Provider = "Provider";
private const string SymbolCompletionItem = "Microsoft.CodeAnalysis.Completion.Providers.SymbolCompletionItem";
private const string SymbolCompletionProvider = "Microsoft.CodeAnalysis.CSharp.Completion.Providers.SymbolCompletionProvider";
private const string SymbolKind = "SymbolKind";
Copy link
Member

Choose a reason for hiding this comment

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

Use nameof?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks!

@DustinCampbell
Copy link
Contributor Author

Note: This isn't quite right. Some tests failing.

@DustinCampbell DustinCampbell merged commit 30a3cf2 into OmniSharp:dev May 19, 2017
@filipw
Copy link
Member

filipw commented May 19, 2017

thanks!

I somehow missed it, sorry. However, it didn't occur for me before - for example this is what I see even before this PR was merged:

screenshot 2017-05-19 10 39 29

@filipw
Copy link
Member

filipw commented May 19, 2017

aaaaah mystery solved!

I never saw this on the original PR because I tested against the version of C# for VS Code which had the magic completion Regex still present (which we remved here dotnet/vscode-csharp@01aa52e). Turns out the purpose of this was to collapse overloads together on the client side.

It absolutely makes sense to do this on the server side correctly instead - thanks @DustinCampbell for fixing 😅

@DustinCampbell DustinCampbell deleted the completion-fixes branch August 30, 2017 19:14
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.

3 participants