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

Introduce configuration for "Go to Symbol in Workspace" #1284

Merged
merged 14 commits into from
Sep 20, 2018

Conversation

dmgonch
Copy link
Contributor

@dmgonch dmgonch commented Sep 3, 2018

Allow configuring number of chars user must to type in for "Go to Symbol in Workspace" command to return any results (default is 0 to preserve existing behavior). The reasoning here is that when lots of projects are loaded by OmniSharp it makes little sense to return all symbols discovered. This change should largely address these issues as well: #1243, dotnet/vscode-csharp#1808.
Additionally allow configuring max number of items returned by "Go to Symbol in Workspace" command. Similarly to the above in a big repo returning pages and pages of items in most cases will be wasteful. But default is 0 which is indicates no limit to preserve existing behavior.

@dmgonch
Copy link
Contributor Author

dmgonch commented Sep 3, 2018

The related commit on vscode side: dotnet/vscode-csharp#2487

Copy link

@rchande rchande left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution! Some thoughts...

Copy link

@rchande rchande left a comment

Choose a reason for hiding this comment

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

Because we're changing the behavior of the FindSymbols endpoint, I think we do need to version it.

@akshita31 akshita31 merged commit 54d9230 into OmniSharp:master Sep 20, 2018
@dmgonch dmgonch deleted the feature/minFindSymbolsFilterLength branch September 21, 2018 04:05
@filipw
Copy link
Member

filipw commented Oct 24, 2018

@rchande @akshita31 I somehow missed this PR before.. what is the reason for MinFilterLength to be introduced on a request? If that comes from the request, then the sender of the request should already know better whether a given value should result in request being sent or not.

this specific check doesn't make much sense, we are validating sender's input, based on sender's condition.. https://github.com/OmniSharp/omnisharp-roslyn/pull/1284/files#diff-030b76f11f6d3bc42e70a3cde2a74be9R24

I think the min length should be implemented on the client side in the VS Code extension, and here we could at best have a global setting that would come from omnisharp.json not from the request

@akshita31
Copy link
Contributor

@filipw Agreed. Apologies that this got missed out somehow.

@dmgonch
Copy link
Contributor Author

dmgonch commented Oct 30, 2018

@filipw Thanks for the feedback!
@akshita31 I will look into making the change unless this is something you are planning to work on right away.

@filipw
Copy link
Member

filipw commented Oct 30, 2018

I already sent a PR here dotnet/vscode-csharp#2625

@dmgonch dmgonch restored the feature/minFindSymbolsFilterLength branch December 23, 2018 05:33
@dmgonch dmgonch deleted the feature/minFindSymbolsFilterLength branch December 23, 2018 05:36
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.

5 participants