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

Fix backslash escape, multiple linters output, .pyenv/versions folder search #920

Merged
merged 101 commits into from
Mar 2, 2018
Merged
Show file tree
Hide file tree
Changes from 92 commits
Commits
Show all changes
101 commits
Select commit Hold shift + click to select a range
7675901
Basic tokenizer
Dec 1, 2017
eb42669
Fixed property names
Dec 1, 2017
2756974
Tests, round I
Dec 1, 2017
c2c1ced
Tests, round II
Dec 2, 2017
a108c96
merge master
Dec 3, 2017
14864a5
tokenizer test
Dec 4, 2017
0ed51d6
Remove temorary change
Dec 4, 2017
51b544c
Fix merge issue
Dec 4, 2017
3cd11e6
Merge conflict
Dec 4, 2017
82e0ad1
Merge conflict
Dec 4, 2017
9295c1a
Completion test
Dec 4, 2017
06eb1a5
Fix last line
Dec 4, 2017
e9db8e0
Fix javascript math
Dec 4, 2017
d12ca03
Merge master
Dec 5, 2017
d8ab041
Make test await for results
Dec 5, 2017
db75cd0
Add license headers
Dec 5, 2017
9ab2c47
Rename definitions to types
Dec 5, 2017
d587485
License headers
Dec 5, 2017
1da5e0a
Merge branch 'master' of https://github.com/Microsoft/vscode-python
Dec 5, 2017
7668cee
Merge branch 'master' of https://github.com/Microsoft/vscode-python
Dec 11, 2017
1ac4932
Fix typo in completion details (typo)
Dec 11, 2017
2aa5a6c
Fix hover test
Dec 12, 2017
5db31bd
Merge branch 'master' of https://github.com/Microsoft/vscode-python
Dec 12, 2017
560d2af
Russian translations
Dec 13, 2017
c71024d
Merge branch 'master' of https://github.com/Microsoft/vscode-python
Dec 13, 2017
31aa087
Update to better translation
Dec 13, 2017
593ae05
Fix typo
Dec 13, 2017
e6d69bb
#70 How to get all parameter info when filling in a function param list
Dec 13, 2017
b5a23d3
Fix #70 How to get all parameter info when filling in a function para…
Dec 14, 2017
cd200f7
Clean up
Dec 14, 2017
7c33228
Clean imports
Dec 14, 2017
c4a6b90
CR feedback
Dec 14, 2017
f85b848
Trim whitespace for test stability
Dec 14, 2017
37c210b
More tests
Dec 15, 2017
61a5650
Better handle no-parameters documentation
Dec 15, 2017
a10305e
Better handle ellipsis and Python3
Dec 15, 2017
bfcae78
Merge branch 'master' of https://github.com/Microsoft/vscode-python
Dec 15, 2017
42a5f79
Merge branch 'master' of https://github.com/Microsoft/vscode-python
Dec 18, 2017
e4ba322
Merge branch 'master' of https://github.com/Microsoft/vscode-python
Jan 8, 2018
7baec1a
Merge branch 'master' of https://github.com/Microsoft/vscode-python
Jan 9, 2018
9cb43e7
#385 Auto-Indentation doesn't work after comment
Jan 9, 2018
5a9c3fd
#141 Auto indentation broken when return keyword involved
Jan 9, 2018
9800c4a
Undo changes
Jan 9, 2018
3205d33
Merge branch 'master' of https://github.com/Microsoft/vscode-python
Jan 11, 2018
c1150d4
Merge branch 'master' of https://github.com/Microsoft/vscode-python
Feb 1, 2018
30519c7
#627 Docstrings for builtin methods are not parsed correctly
Feb 5, 2018
96511cb
reStructuredText converter
Feb 5, 2018
c8670b9
Fix: period is not an operator
Feb 5, 2018
97f232f
Minor fixes
Feb 6, 2018
768bffe
Restructure
Feb 6, 2018
825f16b
Tests
Feb 6, 2018
eb36eef
Tests
Feb 6, 2018
bab4239
Code heuristics
Feb 6, 2018
2a30201
Baselines
Feb 6, 2018
e430ef8
HTML handling
Feb 6, 2018
1afa841
Lists
Feb 7, 2018
6bffb07
State machine
Feb 7, 2018
e436fde
Baselines
Feb 7, 2018
c03e619
Merge branch 'master' of https://github.com/Microsoft/vscode-python
Feb 7, 2018
e52bcff
Squash
Feb 7, 2018
e6b8196
Merge branch 'master' of https://github.com/MikhailArkhipov/vscode-py…
Feb 7, 2018
3a0cfb1
no message
Feb 7, 2018
4616996
Merge branch 'master' of https://github.com/MikhailArkhipov/vscode-py…
Feb 7, 2018
35838b9
Whitespace difference
Feb 7, 2018
656a56b
Merge branch 'master' of https://github.com/Microsoft/vscode-python
Feb 12, 2018
6a10786
Update Jedi to 0.11.1
Feb 12, 2018
d43f097
Enable Travis
Feb 12, 2018
f8db935
Test fixes
Feb 13, 2018
a8dc597
Undo change
Feb 13, 2018
3f5d492
Jedi 0.11 with parser
Feb 13, 2018
f93a0f4
Merge master
Feb 13, 2018
f8eaa93
Undo changes
Feb 13, 2018
e4372c6
Undo changes
Feb 13, 2018
469c8a7
Test fixes
Feb 13, 2018
609bbdd
More tests
Feb 15, 2018
7ea6fda
Tests
Feb 15, 2018
6546892
Fix pylint search
Feb 16, 2018
76af122
Handle quote escapes in strings
Feb 16, 2018
5d4d022
Escapes in strings
Feb 16, 2018
29edac2
CR feedback
Feb 16, 2018
1ee0be2
Discover pylintrc better + tests
Feb 19, 2018
531c1d8
Merge branch 'master' of https://github.com/MikhailArkhipov/vscode-py…
Feb 27, 2018
1969451
Merge master
Feb 27, 2018
33efd6e
Fix .pyenv/versions search
Feb 27, 2018
713983e
Fix multiple linters output
Feb 28, 2018
e64b371
Better handle markdown underscore
Feb 28, 2018
afddcb3
Merge branch 'master' of https://github.com/Microsoft/vscode-python
Feb 28, 2018
73c9617
Test
Feb 28, 2018
eea64de
Fix 916: PyLint checks wrong files
Feb 28, 2018
5a0a553
Test stability
Mar 1, 2018
2c635ba
Try increase timeout
Mar 1, 2018
f37c27c
Make sure linting is enabled in tests
Mar 1, 2018
8fd2d14
Try another way of waiting
Mar 1, 2018
6ac00d8
Simplify
Mar 1, 2018
a9c2708
Fix clear diags on close tests
Mar 1, 2018
5fbc703
Try writing settings directly
Mar 1, 2018
46090c6
Increase timeout
Mar 2, 2018
c54b0ee
Measure test time
Mar 2, 2018
8954180
Measure time
Mar 2, 2018
f6707c1
Simplify
Mar 2, 2018
588313b
Set timeout
Mar 2, 2018
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion src/client/common/markdown/restTextConverter.ts
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,8 @@ export class RestTextConverter {
return text
.replace(/\#/g, '\\#')
.replace(/\*/g, '\\*')
.replace(/\_/g, '\\_');
.replace(/\ _/g, ' \\_')
.replace(/^_/, '\\_');
}

private transformLines(docstring: string): string {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -42,9 +42,11 @@ export class GlobalVirtualEnvironmentsSearchPathProvider implements IVirtualEnvi
folders.push(pyenvRoot);
folders.push(path.join(pyenvRoot, 'versions'));
} else {
// Check if .pyenv/versions is in the list
const pyenvVersions = path.join('.pyenv', 'versions');
if (venvFolders.indexOf('.pyenv') >= 0 && venvFolders.indexOf(pyenvVersions) < 0) {
folders.push(pyenvVersions);
// if .pyenv is in the list, but .pyenv/versions is not, add it.
folders.push(path.join(homedir, pyenvVersions));
}
}
return folders;
Expand Down
4 changes: 2 additions & 2 deletions src/client/linters/linterCommands.ts
Original file line number Diff line number Diff line change
Expand Up @@ -79,9 +79,9 @@ export class LinterCommands implements vscode.Disposable {
}
}

public runLinting(): void {
public runLinting(): Promise<vscode.DiagnosticCollection> {
const engine = this.serviceContainer.get<ILintingEngine>(ILintingEngine);
engine.lintOpenPythonFiles();
return engine.lintOpenPythonFiles();
}

private get settingsUri(): vscode.Uri | undefined {
Expand Down
65 changes: 40 additions & 25 deletions src/client/linters/lintingEngine.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import * as path from 'path';
import * as vscode from 'vscode';
import { IDocumentManager, IWorkspaceService } from '../common/application/types';
import { LinterErrors, PythonLanguage, STANDARD_OUTPUT_CHANNEL } from '../common/constants';
import { IFileSystem } from '../common/platform/types';
import { IConfigurationService, IOutputChannel } from '../common/types';
import { IServiceContainer } from '../ioc/types';
import { JupyterProvider } from '../jupyter/provider';
Expand Down Expand Up @@ -40,6 +41,7 @@ export class LintingEngine implements ILintingEngine {
private diagnosticCollection: vscode.DiagnosticCollection;
private pendingLintings = new Map<string, vscode.CancellationTokenSource>();
private outputChannel: vscode.OutputChannel;
private fileSystem: IFileSystem;

constructor(@inject(IServiceContainer) private serviceContainer: IServiceContainer) {
this.documentHasJupyterCodeCells = (a, b) => Promise.resolve(false);
Expand All @@ -48,35 +50,23 @@ export class LintingEngine implements ILintingEngine {
this.configurationService = serviceContainer.get<IConfigurationService>(IConfigurationService);
this.outputChannel = serviceContainer.get<vscode.OutputChannel>(IOutputChannel, STANDARD_OUTPUT_CHANNEL);
this.linterManager = serviceContainer.get<ILinterManager>(ILinterManager);
this.fileSystem = serviceContainer.get<IFileSystem>(IFileSystem);
this.diagnosticCollection = vscode.languages.createDiagnosticCollection('python');
}

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) {

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

Yes

await this.lintDocument(document, 'auto');
this.lintDocument(document, 'auto').ignoreErrors();
}
});
return this.diagnosticCollection;
}

public async lintDocument(document: vscode.TextDocument, trigger: LinterTrigger): Promise<void> {
public async lintDocument(document: vscode.TextDocument, trigger: LinterTrigger): Promise<vscode.DiagnosticCollection> {

Choose a reason for hiding this comment

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

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.

Choose a reason for hiding this comment

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

Shoudn't the method public async lintOpenPythonFiles(): Promise<vscode.DiagnosticCollection> collect everything and send a consolidated view?

Copy link
Author

Choose a reason for hiding this comment

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

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.

// Check if we need to lint this document
const workspaceFolder = this.workspace.getWorkspaceFolder(document.uri);
const workspaceRootPath = (workspaceFolder && typeof workspaceFolder.uri.fsPath === 'string') ? workspaceFolder.uri.fsPath : undefined;
const relativeFileName = typeof workspaceRootPath === 'string' ? path.relative(workspaceRootPath, document.fileName) : document.fileName;
const settings = this.configurationService.getSettings(document.uri);
if (document.languageId !== PythonLanguage.language) {
return;
}
if (!this.linterManager.isLintingEnabled(document.uri)) {
this.diagnosticCollection.set(document.uri, []);
}
const ignoreMinmatches = settings.linting.ignorePatterns.map(pattern => {
return new Minimatch(pattern);
});

if (ignoreMinmatches.some(matcher => matcher.match(document.fileName) || matcher.match(relativeFileName))) {
return;
if (!await this.shouldLintDocument(document)) {
return this.diagnosticCollection;
}

if (this.pendingLintings.has(document.uri.fsPath)) {
Expand Down Expand Up @@ -107,14 +97,14 @@ export class LintingEngine implements ILintingEngine {
// linters will resolve asynchronously - keep a track of all
// diagnostics reported as them come in.
let diagnostics: vscode.Diagnostic[] = [];
const settings = this.configurationService.getSettings(document.uri);

for (const p of promises) {
const msgs = await p;
if (cancelToken.token.isCancellationRequested) {
break;
}

diagnostics = [];
if (this.isDocumentOpen(document.uri)) {
// Build the message and suffix the message with the name of the linter used.
for (const m of msgs) {
Expand All @@ -123,17 +113,17 @@ export class LintingEngine implements ILintingEngine {
(m.code === LinterErrors.pylint.InvalidSyntax ||
m.code === LinterErrors.prospector.InvalidSyntax ||
m.code === LinterErrors.flake8.InvalidSyntax)) {
return;
continue;
}
diagnostics.push(this.createDiagnostics(m, document));
}

// Limit the number of messages to the max value.
diagnostics = diagnostics.filter((value, index) => index <= settings.linting.maxNumberOfProblems);
}
// Set all diagnostics found in this pass, as this method always clears existing diagnostics.
this.diagnosticCollection.set(document.uri, diagnostics);
}
// Set all diagnostics found in this pass, as this method always clears existing diagnostics.
this.diagnosticCollection.set(document.uri, diagnostics);
return this.diagnosticCollection;
}

// tslint:disable-next-line:no-any
Expand Down Expand Up @@ -175,4 +165,29 @@ export class LintingEngine implements ILintingEngine {
diagnostic.source = message.provider;
return diagnostic;
}

private async shouldLintDocument(document: vscode.TextDocument): Promise<boolean> {
if (!this.linterManager.isLintingEnabled(document.uri)) {
this.diagnosticCollection.set(document.uri, []);
return false;
}

if (document.languageId !== PYTHON.language) {
return false;
}

const workspaceFolder = this.workspace.getWorkspaceFolder(document.uri);
const workspaceRootPath = (workspaceFolder && typeof workspaceFolder.uri.fsPath === 'string') ? workspaceFolder.uri.fsPath : undefined;
const relativeFileName = typeof workspaceRootPath === 'string' ? path.relative(workspaceRootPath, document.fileName) : document.fileName;

const settings = this.configurationService.getSettings(document.uri);
const ignoreMinmatches = settings.linting.ignorePatterns.map(pattern => new Minimatch(pattern));
if (ignoreMinmatches.some(matcher => matcher.match(document.fileName) || matcher.match(relativeFileName))) {
return false;
}
if (document.uri.scheme !== 'file' || !document.uri.fsPath) {
return false;
}
return await this.fileSystem.fileExistsAsync(document.uri.fsPath);
}
}
4 changes: 2 additions & 2 deletions src/client/linters/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -61,8 +61,8 @@ export enum LintMessageSeverity {

export const ILintingEngine = Symbol('ILintingEngine');
export interface ILintingEngine {
lintOpenPythonFiles(): void;
lintDocument(document: vscode.TextDocument, trigger: LinterTrigger): Promise<void>;
lintOpenPythonFiles(): Promise<vscode.DiagnosticCollection>;
lintDocument(document: vscode.TextDocument, trigger: LinterTrigger): Promise<vscode.DiagnosticCollection>;
// tslint:disable-next-line:no-any
linkJupiterExtension(jupiter: vscode.Extension<any> | undefined): Promise<void>;
}
29 changes: 11 additions & 18 deletions src/client/providers/linterProvider.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,14 +6,13 @@ import * as vscode from 'vscode';
import { ConfigurationTarget, Uri, workspace } from 'vscode';
import { IDocumentManager } from '../common/application/types';
import { ConfigSettingMonitor } from '../common/configSettingMonitor';
import { isTestExecution } from '../common/constants';
import { IFileSystem } from '../common/platform/types';
import { IConfigurationService } from '../common/types';
import { IInterpreterService } from '../interpreter/contracts';
import { IServiceContainer } from '../ioc/types';
import { ILinterManager, ILintingEngine } from '../linters/types';

const uriSchemesToIgnore = ['git', 'showModifications', 'svn'];

export class LinterProvider implements vscode.Disposable {
private diagnosticCollection: vscode.DiagnosticCollection;
private context: vscode.ExtensionContext;
Expand Down Expand Up @@ -46,6 +45,12 @@ export class LinterProvider implements vscode.Disposable {

this.configMonitor = new ConfigSettingMonitor('linting');
this.configMonitor.on('change', this.lintSettingsChangedHandler.bind(this));

// On workspace reopen we don't get `onDocumentOpened` since it is first opened
// and then the extension is activated. So schedule linting pass now.
if (!isTestExecution) {
setTimeout(() => this.engine.lintOpenPythonFiles().ignoreErrors(), 2000);
}
}

public get diagnostics(): vscode.DiagnosticCollection {
Expand All @@ -63,35 +68,23 @@ export class LinterProvider implements vscode.Disposable {

private lintSettingsChangedHandler(configTarget: ConfigurationTarget, wkspaceOrFolder: Uri) {
if (configTarget === ConfigurationTarget.Workspace) {
this.engine.lintOpenPythonFiles();
this.engine.lintOpenPythonFiles().ignoreErrors();
return;
}
// Look for python files that belong to the specified workspace folder.
workspace.textDocuments.forEach(async document => {
const wkspaceFolder = workspace.getWorkspaceFolder(document.uri);
if (wkspaceFolder && wkspaceFolder.uri.fsPath === wkspaceOrFolder.fsPath) {
await this.engine.lintDocument(document, 'auto');
this.engine.lintDocument(document, 'auto').ignoreErrors();
}
});
}

private async onDocumentOpened(document: vscode.TextDocument): Promise<void> {
const settings = this.configuration.getSettings(document.uri);
if (document.languageId !== 'python' || !settings.linting.enabled) {
return;
}
// Exclude files opened by vscode when showing a diff view.
if (uriSchemesToIgnore.indexOf(document.uri.scheme) >= 0) {
return;
}
if (!document.uri.path ||
(path.basename(document.uri.path) === document.uri.path && !await this.fs.fileExistsAsync(document.uri.path))) {
return;
}
private onDocumentOpened(document: vscode.TextDocument): void {
this.engine.lintDocument(document, 'auto').ignoreErrors();
}

private onDocumentSaved(document: vscode.TextDocument) {
private onDocumentSaved(document: vscode.TextDocument): void {
const settings = this.configuration.getSettings(document.uri);
if (document.languageId === 'python' && settings.linting.enabled && settings.linting.lintOnSave) {
this.engine.lintDocument(document, 'save').ignoreErrors();
Expand Down
2 changes: 1 addition & 1 deletion src/test/interpreters/venv.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ suite('Virtual environments', () => {

let paths = pathProvider.getSearchPaths();
let expected = folders.map(item => path.join(homedir, item));
expected.push(path.join('.pyenv', 'versions'));
expected.push(path.join(homedir, '.pyenv', 'versions'));

expect(paths).to.deep.equal(expected, 'Global search folder list is incorrect.');

Expand Down
32 changes: 29 additions & 3 deletions src/test/linters/lint.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ import { Product } from '../../client/common/installer/productInstaller';
import { IConfigurationService, IOutputChannel } from '../../client/common/types';
import { LinterManager } from '../../client/linters/linterManager';
import { ILinterManager, ILintMessage, LintMessageSeverity } from '../../client/linters/types';
import { deleteFile, PythonSettingKeys, rootWorkspaceUri } from '../common';
import { deleteFile, PythonSettingKeys, rootWorkspaceUri, sleep } from '../common';
import { closeActiveWindows, initialize, initializeTest, IS_MULTI_ROOT_TEST } from '../initialize';
import { MockOutputChannel } from '../mockClasses';
import { UnitTestIocContainer } from '../unittests/serviceRegistry';
Expand Down Expand Up @@ -99,7 +99,7 @@ suite('Linting', () => {

suiteSetup(initialize);
setup(async () => {
initializeDI();
await initializeDI();
await initializeTest();
await resetSettings();
});
Expand All @@ -112,7 +112,7 @@ suite('Linting', () => {
await deleteFile(path.join(workspaceUri.fsPath, '.pydocstyle'));
});

function initializeDI() {
async function initializeDI() {
ioc = new UnitTestIocContainer();
ioc.registerCommonTypes(false);
ioc.registerProcessTypes();
Expand All @@ -122,6 +122,7 @@ suite('Linting', () => {

linterManager = new LinterManager(ioc.serviceContainer);
configService = ioc.serviceContainer.get<IConfigurationService>(IConfigurationService);
await linterManager.enableLintingAsync(true);
}

async function resetSettings() {
Expand Down Expand Up @@ -250,4 +251,29 @@ suite('Linting', () => {
await configService.updateSettingAsync('linting.pylintUseMinimalCheckers', false, workspaceUri);
await testEnablingDisablingOfLinter(Product.pylint, true, file);
});
test('Multiple linters', async () => {
await linterManager.setActiveLintersAsync([Product.pylint, Product.flake8]);

const document = await vscode.workspace.openTextDocument(path.join(pythoFilesPath, 'print.py'));
const collection = await vscode.commands.executeCommand('python.runLinting') as vscode.DiagnosticCollection;
assert.notEqual(collection, undefined, 'python.runLinting did not return valid diagnostics collection.');

const ready = await waitForCondition(() => collection!.has(document.uri) && collection!.get(document.uri)!.length >= 3);
assert.equal(ready, true, 'Timeout expired but linting results are not available still.');

const messages = collection!.get(document.uri);
assert.notEqual(messages!.filter(x => x.source === 'pylint').length, 0, 'No pylint messages.');
assert.notEqual(messages!.filter(x => x.source === 'flake8').length, 0, 'No flake8 messages.');
});

async function waitForCondition(predicate: () => boolean, interval = 1000, maxAttempts = 15): Promise<boolean> {
return new Promise<boolean>(async (resolve) => {
let retries = 0;
while (!predicate() && retries < maxAttempts) {
await sleep(1000);
retries += 1;
}
resolve(retries < maxAttempts);
});
}
});
Loading