Fix backslash escape, multiple linters output, .pyenv/versions folder search #920
Conversation
|
@MikhailArkhipov We're missing the following check in // Exclude files opened by vscode when showing a diff view.
if (uriSchemesToIgnore.indexOf(document.uri.scheme) >= 0) {
return;
}Here are the file schemes that need to be excluded (found in |
|
Sure. We probably want all checks from |
|
@MikhailArkhipov Yes, the issue numbers are linked in the message, but the title isn't and that's what I see in the GitHub notifications to determine if this PR is something I need to look at. |
Codecov Report
@@ Coverage Diff @@
## master #920 +/- ##
=========================================
- Coverage 64.34% 64.04% -0.3%
=========================================
Files 260 260
Lines 12042 12025 -17
Branches 2133 2131 -2
=========================================
- Hits 7748 7701 -47
- Misses 4285 4315 +30
Partials 9 9
Continue to review full report at Codecov.
|
|
Fixed #916 + tests. Need to watch one test stability on AppVeyor |
src/client/linters/lintingEngine.ts
Outdated
| public lintOpenPythonFiles(): void { | ||
| this.documents.textDocuments.forEach(async document => { | ||
| public async lintOpenPythonFiles(): Promise<vscode.DiagnosticCollection> { | ||
| this.documents.textDocuments.forEach(document => { | ||
| if (document.languageId === PythonLanguage.language) { |
There was a problem hiding this comment.
This check is no longer necessary as you now have shouldLintDocument method that does the necessary checking. Besides this only checks one condition and could be misleading about the logic.
src/client/linters/lintingEngine.ts
Outdated
| } | ||
|
|
||
| public async lintDocument(document: vscode.TextDocument, trigger: LinterTrigger): Promise<void> { | ||
| public async lintDocument(document: vscode.TextDocument, trigger: LinterTrigger): Promise<vscode.DiagnosticCollection> { |
There was a problem hiding this comment.
This method doesn't feel right. Its modifying the underlying collection and returning the same thing.
Feels unnecessary to return the whole collection.
E.g. if you line a document, you'd expect to get errors related to that document, not all documents.
There was a problem hiding this comment.
Shoudn't the method public async lintOpenPythonFiles(): Promise<vscode.DiagnosticCollection> collect everything and send a consolidated view?
There was a problem hiding this comment.
This is for tests only, to get the active collection via vscode.commands.executeCommand('python.runLinting'). Currently linting of documents is completely non-blocking and I didn't want to wait until the entire collection is populated. However, it is doable.
|
OK, |
|
You can increase timeout for a single test as follows: test('Ensure non-existing files are not linted', async function () {
this.timeout(40000) |
|
Hmm, taking 35 seconds for linting, sounds something is wrong, that's an awfully long time. |
|
Local result: Local What is VM configuration on the test machine? RAM/Cores? |
|
Yay 👍 all tests pass |
src/test/index.ts
Outdated
| @@ -21,7 +21,7 @@ const grep = IS_CI_SERVER && IS_CI_SERVER_TEST_DEBUGGER ? 'Debug' : undefined; | |||
| const options: MochaSetupOptions & { retries: number } = { | |||
| ui: 'tdd', | |||
| useColors: true, | |||
| timeout: 25000, | |||
| timeout: 35000, | |||
There was a problem hiding this comment.
Please could we increase the timeout of the individual test rather than doing this for all.
The problem with this approach is, it affects all tests.
This was necessary earlier, when most of the tests were running actual python code.
There was a problem hiding this comment.
Here's a sample:
test('Ensure non-existing files are not linted', async function () {
this.timeout(40000)
Fixes #911 - Backslash escape for underscores in Markdown docstrings
Fixes #913 - Multiple linters not working as expected
Fixes search for
.pyenv/versionsfor #847.pyenv/versionsto make it into the absolute path