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

Allow client to specify symbol filter for FindSymbols Endpoint. #1823

Merged
merged 6 commits into from
Jul 10, 2020

Conversation

juliuszint
Copy link
Contributor

FindSymbols Endpoint now exposes a SymbolFilter Property. This enables the Client to specify what Symbols he is interested in. Previously SymbolFilter.TypeAndMember was used by default. Especially in larger projects this results in poor performance.

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,
Copy link
Member

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

Copy link
Contributor Author

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.

@filipw
Copy link
Member

filipw commented Jun 6, 2020

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);
Copy link
Member

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

Copy link
Contributor Author

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.

@juliuszint
Copy link
Contributor Author

juliuszint commented Jun 6, 2020

this is a good idea, thanks. however, which client currently needs symbol filter?

I would implement it in omnisharp-vim. I use it on a daily basis with a project where the OmniSharpFindSymbols command takes somewhere around 6 seconds. With the filter set to Type it returns in less than a second.

adding a unit tests to ensure the SymbolFilter gets used.
Copy link
Member

@filipw filipw left a comment

Choose a reason for hiding this comment

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

LGTM

@filipw
Copy link
Member

filipw commented Jun 19, 2020

@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

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.

:shipit:

@filipw
Copy link
Member

filipw commented Jul 10, 2020

thank you for the contribution

@filipw filipw merged commit 05a63c0 into OmniSharp:master Jul 10, 2020
@juliuszint juliuszint deleted the feature-configurablesymbolfilter branch July 11, 2020 10:30
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.

4 participants