-
Notifications
You must be signed in to change notification settings - Fork 420
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
Allow client to specify symbol filter for FindSymbols Endpoint. #1823
Allow client to specify symbol filter for FindSymbols Endpoint. #1823
Conversation
Client to specify what symbols he is interested in. Previously the SymbolFilter.TypeAndMember was used by default. Especially in larger projects this results in poor performance.
Namespace = 1, | ||
Type = 2, | ||
Member = 4, | ||
TypeAndMember = 6, |
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.
you can use thee same syntax as in Roslyn e.g. TypeAndMember = Type | Member
or All = Namespace | Type | Member
to make it more readable
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.
Will do so. Thanks for the feedback.
this is a good idea, thanks. however, which client currently needs symbol filter? AFAIK all clients use "go to anything" type of functionality like VS or R# has, that's why the default behavior was what it was. Also, you need to add some tests here https://github.com/OmniSharp/omnisharp-roslyn/blob/master/tests/OmniSharp.Roslyn.CSharp.Tests/FindSymbolsFacts.cs |
@@ -26,8 +26,9 @@ public override Task<QuickFixResponse> Handle(FindSymbolsRequest request) | |||
return Task.FromResult(new QuickFixResponse { QuickFixes = Array.Empty<QuickFix>() }); | |||
} | |||
|
|||
var symbolFilter = (Microsoft.CodeAnalysis.SymbolFilter)(request?.SymbolFilter ?? OmniSharpSymbolFilter.TypeAndMember); |
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.
you should use the same pattern as below (request?.SymbolFilter).GetValueOrDefault(OmniSharpSymbolFilter.TypeAndMember)
for consistency
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.
When i use using Microsoft.CodeAnalysis;
there will be a Type ambigutity. I could change it to: using SymbolFilter = Microsoft.CodeAnalysis.SymbolFilter
if you prefer this.
I would implement it in omnisharp-vim. I use it on a daily basis with a project where the |
adding a unit tests to ensure the SymbolFilter gets used.
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.
LGTM
@JoeRobich do you see any use for this in the VS Code extension? you could add "go to type" alongside the "go to anything". in either case, the change is non-breaking so existing use cases are not affected |
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.
thank you for the contribution |
FindSymbols Endpoint now exposes a
SymbolFilter
Property. This enables the Client to specify what Symbols he is interested in. PreviouslySymbolFilter.TypeAndMember
was used by default. Especially in larger projects this results in poor performance.