Propagate files exclusion to the language server #1977
Conversation
src/client/activation/analysis.ts
Outdated
| @@ -3,15 +3,14 @@ | |||
|
|
|||
| import { inject, injectable } from 'inversify'; | |||
| import * as path from 'path'; | |||
| import { ExtensionContext, OutputChannel } from 'vscode'; | |||
| import { ExtensionContext, OutputChannel, workspace } from 'vscode'; | |||
There was a problem hiding this comment.
Please use IWorkspaceService
You can remove ExtensionContext and use IExtensionContext (type is already imported).
src/client/activation/analysis.ts
Outdated
| @@ -46,6 +45,7 @@ export class AnalysisExtensionActivator implements IExtensionActivator { | |||
| private languageClient: LanguageClient | undefined; | |||
| private readonly context: ExtensionContext; | |||
| private interpreterHash: string = ''; | |||
| private loadExtensionArgs; | |||
There was a problem hiding this comment.
type definition for this property is missing.
This property needs to be marked as nullable (private loadExtensionArgs?)
src/client/activation/analysis.ts
Outdated
| } | ||
|
|
||
| private getPythonExcludeSection(setting: string, list: string[]): void { | ||
| const paths = workspace.getConfiguration('python', null).get<string[]>(setting); |
There was a problem hiding this comment.
Please pass a resource identifier around instead of using null. This way, later all we need to do is provide the resource, or please add a TODO.
There was a problem hiding this comment.
We'll need to figure out resource here for the multiroot case #1149. In the current incarnation LS only supports single root. I guess first workspaceFolder will do for now.
| textDocument = await workspace.openTextDocument(file); | ||
| await activated; | ||
| await window.showTextDocument(textDocument); | ||
| await commands.executeCommand('vscode.executeCompletionItemProvider', textDocument.uri, new Position(0, 0)); |
There was a problem hiding this comment.
whats the purpose of invoking this command in the openFile function?
There was a problem hiding this comment.
Making sure LS completes loading files and analysis.
|
|
||
| test('Default exclusions', async () => { | ||
| await openFile(fileOne); | ||
| const diag = languages.getDiagnostics(); |
There was a problem hiding this comment.
Isn't it better to write a unit test here to confirm file exclusions get passed to LSP.
And then have .NET tests to validate exclusions.
I.e. can avoid integration tests.
There was a problem hiding this comment.
Actually, this found pretty good bug in LS - race condition when diagnostic was published before initialization completed. So I'd rather have a real thing.
There was a problem hiding this comment.
We don't have .NET unit tests and not planning to have them for some time. Besides, we wouldn't be able to detect the race condition in mocks and separate tests. From LS point of view everything is fine, it is free threaded. It is VSC that has problem accepting incoming notifications.
There was a problem hiding this comment.
Thats fine, please leave these tests.
| } | ||
|
|
||
| async function setSetting(name: string, value: {} | undefined): Promise<void> { | ||
| await configService.updateSettingAsync(name, value, undefined, ConfigurationTarget.Global); |
There was a problem hiding this comment.
Would be great if we could avoid integration tests where we update VSC settings. VSC have had a few bugs are updating of settings. I've noticed that some of our tests still fail (flaky) due to VSC bugs.
There was a problem hiding this comment.
Setting test (custom folder) is not strictly needed, I can nix it
Codecov Report
@@ Coverage Diff @@
## master #1977 +/- ##
==========================================
- Coverage 74.51% 74.35% -0.16%
==========================================
Files 285 287 +2
Lines 13364 13511 +147
Branches 2390 2408 +18
==========================================
+ Hits 9958 10046 +88
- Misses 3268 3332 +64
+ Partials 138 133 -5
Continue to review full report at Codecov.
|
Fixes #1861
files.exclude,files.watcherExclude,search.exclude,python.linting.ignorePatterns,python.workspaceSymbols.exclusionPatternsThis pull request: