Don't duplicate find symbols results from C# files #1282
Conversation
| : true; | ||
|
|
||
| return Workspace.CurrentSolution.FindSymbols(isMatch, | ||
| p => p.Name.EndsWith(".cake", StringComparison.OrdinalIgnoreCase)); |
There was a problem hiding this comment.
Could we reuse the extension as defined in the project system?
There was a problem hiding this comment.
Maybe the project system and this handler should point to the same constant?
| @@ -30,57 +27,8 @@ public async Task<QuickFixResponse> Handle(FindSymbolsRequest request = null) | |||
| ? candidate.IsValidCompletionFor(request.Filter) | |||
| : true; | |||
|
|
|||
| return await FindSymbols(isMatch); | |||
| return await _workspace.CurrentSolution.FindSymbols(isMatch, | |||
| p => !p.Name.EndsWith(".cake", StringComparison.OrdinalIgnoreCase)); | |||
There was a problem hiding this comment.
Could we try avoiding to use .cake extension in OmniSharp.Roslyn.CSharp, instead allow only .cs and .csx..? Would be difficult to get from project system, since OmniSharp.Roslyn.CSharp doesn't have (and shouldn't) have a dependency on OmniSharp.Scripting.
There was a problem hiding this comment.
Yeah, this feels weird. It would certainly fix the issue, but it feels a bit dirty and easily broken.
There was a problem hiding this comment.
Yeah I think being explicit about the file types supported makes sense, instead of excluding things. If for example we ever add vb support, then this will start to leak again.
|
@mholo65 Done; thanks. It does kind of look like this has been present for as long as cake... |
|
@mholo65 @DustinCampbell Another approach could be to simply remove the "cake" flavored handler. Ctrl-t in VS Code doesn't specify a language, so the C# extension doesn't pass one to OmniSharp, thus running both handlers. However, the symbol provider (https://code.visualstudio.com/docs/extensionAPI/vscode-api#DocumentSymbolProvider) gets access to the current document, so we could specify a language in the request based on the current document. Of course, that means you could only search for Cake symbols from a Cake file and vice versa. Thoughts? |
| ? candidate.IsValidCompletionFor(request.Filter) | ||
| : true; | ||
|
|
||
| return Workspace.CurrentSolution.FindSymbols(isMatch, |
There was a problem hiding this comment.
Should the FindSymbols extension method just take the file extension? It seems error prone to always remember to specify StringComparison.OrdinalIgnoreCase.
| @@ -28,7 +28,7 @@ public async Task<QuickFixResponse> Handle(FindSymbolsRequest request = null) | |||
| : true; | |||
|
|
|||
| return await _workspace.CurrentSolution.FindSymbols(isMatch, | |||
| p => !p.Name.EndsWith(".cake", StringComparison.OrdinalIgnoreCase)); | |||
| p => p.Name.EndsWith(".csproj", StringComparison.OrdinalIgnoreCase)); | |||
There was a problem hiding this comment.
Correct me if I’m wrong, but this will stop finding symbols in.csx files also?
| @@ -119,7 +119,7 @@ private ProjectInfo CreateMiscFilesProject(string language) | |||
| var projectInfo = ProjectInfo.Create( | |||
| id: ProjectId.CreateNewId(), | |||
| version: VersionStamp.Create(), | |||
| name: "MiscellaneousFiles", | |||
| name: "MiscellaneousFiles.csproj", | |||
There was a problem hiding this comment.
What if someone already has a project called MiscellaneousFiles.csproj in their workspace? Would a GUID or some obscure name be beneficial to avoid a name clash?
|
|
||
| return new SymbolLocation | ||
| var csprojSymbols = await _workspace.CurrentSolution.FindSymbols(isMatch, ".csproj"); | ||
| var projectJsonSymbols = await _workspace.CurrentSolution.FindSymbols(isMatch, ".json"); |
There was a problem hiding this comment.
What about .csx? I think it should be here.
I think there should be a single FindSymbolsHandler that lives in the |
|
In the interest of being able to release today, I'm going to merge this PR. I filed #1287 to track cleaning this up further. |
Fixes #1281