From 6c10e9bd5bfa4430cf584683e5741c9b6c0428bd Mon Sep 17 00:00:00 2001 From: Don Jayamanne Date: Wed, 21 Mar 2018 17:02:08 -0700 Subject: [PATCH 1/4] :bug: format file names when passing as command line args --- src/client/linters/flake8.ts | 6 +++--- src/client/linters/mypy.ts | 6 +++--- src/client/linters/pep8.ts | 6 +++--- src/client/linters/prospector.ts | 6 +++--- src/client/linters/pydocstyle.ts | 7 ++++--- src/client/linters/pylama.ts | 6 +++--- src/client/linters/pylint.ts | 6 +++--- 7 files changed, 22 insertions(+), 21 deletions(-) diff --git a/src/client/linters/flake8.ts b/src/client/linters/flake8.ts index efc00becf9a9..494174e15e5d 100644 --- a/src/client/linters/flake8.ts +++ b/src/client/linters/flake8.ts @@ -1,5 +1,5 @@ -import { OutputChannel } from 'vscode'; -import { CancellationToken, TextDocument } from 'vscode'; +import { CancellationToken, OutputChannel, TextDocument } from 'vscode'; +import '../common/extensions'; import { Product } from '../common/types'; import { IServiceContainer } from '../ioc/types'; import { BaseLinter } from './baseLinter'; @@ -13,7 +13,7 @@ export class Flake8 extends BaseLinter { } protected async runLinter(document: TextDocument, cancellation: CancellationToken): Promise { - const messages = await this.run(['--format=%(row)d,%(col)d,%(code).1s,%(code)s:%(text)s', document.uri.fsPath], document, cancellation); + const messages = await this.run(['--format=%(row)d,%(col)d,%(code).1s,%(code)s:%(text)s', document.uri.fsPath.fileToCommandArgument()], document, cancellation); messages.forEach(msg => { msg.severity = this.parseMessagesSeverity(msg.type, this.pythonSettings.linting.flake8CategorySeverity); }); diff --git a/src/client/linters/mypy.ts b/src/client/linters/mypy.ts index 15c8046a08d3..1064488700d5 100644 --- a/src/client/linters/mypy.ts +++ b/src/client/linters/mypy.ts @@ -1,5 +1,5 @@ -import { OutputChannel } from 'vscode'; -import { CancellationToken, TextDocument } from 'vscode'; +import { CancellationToken, OutputChannel, TextDocument } from 'vscode'; +import '../common/extensions'; import { Product } from '../common/types'; import { IServiceContainer } from '../ioc/types'; import { BaseLinter } from './baseLinter'; @@ -13,7 +13,7 @@ export class MyPy extends BaseLinter { } protected async runLinter(document: TextDocument, cancellation: CancellationToken): Promise { - const messages = await this.run([document.uri.fsPath], document, cancellation, REGEX); + const messages = await this.run([document.uri.fsPath.fileToCommandArgument()], document, cancellation, REGEX); messages.forEach(msg => { msg.severity = this.parseMessagesSeverity(msg.type, this.pythonSettings.linting.mypyCategorySeverity); msg.code = msg.type; diff --git a/src/client/linters/pep8.ts b/src/client/linters/pep8.ts index 24bcaa9abeb2..e13d6c91c2b0 100644 --- a/src/client/linters/pep8.ts +++ b/src/client/linters/pep8.ts @@ -1,5 +1,5 @@ -import { OutputChannel } from 'vscode'; -import { CancellationToken, TextDocument } from 'vscode'; +import { CancellationToken, OutputChannel, TextDocument } from 'vscode'; +import '../common/extensions'; import { Product } from '../common/types'; import { IServiceContainer } from '../ioc/types'; import { BaseLinter } from './baseLinter'; @@ -13,7 +13,7 @@ export class Pep8 extends BaseLinter { } protected async runLinter(document: TextDocument, cancellation: CancellationToken): Promise { - const messages = await this.run(['--format=%(row)d,%(col)d,%(code).1s,%(code)s:%(text)s', document.uri.fsPath], document, cancellation); + const messages = await this.run(['--format=%(row)d,%(col)d,%(code).1s,%(code)s:%(text)s', document.uri.fsPath.fileToCommandArgument()], document, cancellation); messages.forEach(msg => { msg.severity = this.parseMessagesSeverity(msg.type, this.pythonSettings.linting.pep8CategorySeverity); }); diff --git a/src/client/linters/prospector.ts b/src/client/linters/prospector.ts index 930fa91c458a..5642c5433848 100644 --- a/src/client/linters/prospector.ts +++ b/src/client/linters/prospector.ts @@ -1,5 +1,5 @@ -import { OutputChannel } from 'vscode'; -import { CancellationToken, TextDocument } from 'vscode'; +import { CancellationToken, OutputChannel, TextDocument } from 'vscode'; +import '../common/extensions'; import { Product } from '../common/types'; import { IServiceContainer } from '../ioc/types'; import { BaseLinter } from './baseLinter'; @@ -28,7 +28,7 @@ export class Prospector extends BaseLinter { } protected async runLinter(document: TextDocument, cancellation: CancellationToken): Promise { - return await this.run(['--absolute-paths', '--output-format=json', document.uri.fsPath], document, cancellation); + return this.run(['--absolute-paths', '--output-format=json', document.uri.fsPath], document, cancellation); } protected async parseMessages(output: string, document: TextDocument, token: CancellationToken, regEx: string) { let parsedData: IProspectorResponse; diff --git a/src/client/linters/pydocstyle.ts b/src/client/linters/pydocstyle.ts index f0b05bb16726..c22944f421d6 100644 --- a/src/client/linters/pydocstyle.ts +++ b/src/client/linters/pydocstyle.ts @@ -1,6 +1,6 @@ import * as path from 'path'; -import { OutputChannel } from 'vscode'; -import { CancellationToken, TextDocument } from 'vscode'; +import { CancellationToken, OutputChannel, TextDocument } from 'vscode'; +import '../common/extensions'; import { Product } from '../common/types'; import { IServiceContainer } from '../ioc/types'; import { IS_WINDOWS } from './../common/utils'; @@ -13,7 +13,7 @@ export class PyDocStyle extends BaseLinter { } protected async runLinter(document: TextDocument, cancellation: CancellationToken): Promise { - const messages = await this.run([document.uri.fsPath], document, cancellation); + const messages = await this.run([document.uri.fsPath.fileToCommandArgument()], document, cancellation); // All messages in pep8 are treated as warnings for now. messages.forEach(msg => { msg.severity = LintMessageSeverity.Warning; @@ -61,6 +61,7 @@ export class PyDocStyle extends BaseLinter { const trmmedSourceLine = sourceLine.trim(); const sourceStart = sourceLine.indexOf(trmmedSourceLine); + // tslint:disable-next-line:no-object-literal-type-assertion return { code: code, message: message, diff --git a/src/client/linters/pylama.ts b/src/client/linters/pylama.ts index ab29fd9c55ec..ef66bc5446c3 100644 --- a/src/client/linters/pylama.ts +++ b/src/client/linters/pylama.ts @@ -1,5 +1,5 @@ -import { OutputChannel } from 'vscode'; -import { CancellationToken, TextDocument } from 'vscode'; +import { CancellationToken, OutputChannel, TextDocument } from 'vscode'; +import '../common/extensions'; import { Product } from '../common/types'; import { IServiceContainer } from '../ioc/types'; import { BaseLinter } from './baseLinter'; @@ -14,7 +14,7 @@ export class PyLama extends BaseLinter { } protected async runLinter(document: TextDocument, cancellation: CancellationToken): Promise { - const messages = await this.run(['--format=parsable', document.uri.fsPath], document, cancellation, REGEX); + const messages = await this.run(['--format=parsable', document.uri.fsPath.fileToCommandArgument()], document, cancellation, REGEX); // All messages in pylama are treated as warnings for now. messages.forEach(msg => { msg.severity = LintMessageSeverity.Warning; diff --git a/src/client/linters/pylint.ts b/src/client/linters/pylint.ts index 4111c17f83bc..4998fe22a46a 100644 --- a/src/client/linters/pylint.ts +++ b/src/client/linters/pylint.ts @@ -4,8 +4,8 @@ import * as os from 'os'; import * as path from 'path'; -import { OutputChannel } from 'vscode'; -import { CancellationToken, TextDocument } from 'vscode'; +import { CancellationToken, OutputChannel, TextDocument } from 'vscode'; +import '../common/extensions'; import { IFileSystem, IPlatformService } from '../common/platform/types'; import { Product } from '../common/types'; import { IServiceContainer } from '../ioc/types'; @@ -70,7 +70,7 @@ export class Pylint extends BaseLinter { '--msg-template=\'{line},{column},{category},{msg_id}:{msg}\'', '--reports=n', '--output-format=text', - uri.fsPath + uri.fsPath.fileToCommandArgument() ]; const messages = await this.run(minArgs.concat(args), document, cancellation); messages.forEach(msg => { From b43cb076f01ce6bf90b2083dc815c0cb3e7e7a74 Mon Sep 17 00:00:00 2001 From: Don Jayamanne Date: Thu, 22 Mar 2018 13:40:52 -0700 Subject: [PATCH 2/4] :bug: fix header check --- gulpfile.js | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) diff --git a/gulpfile.js b/gulpfile.js index 582908f4468d..a1679e5da6bf 100644 --- a/gulpfile.js +++ b/gulpfile.js @@ -24,6 +24,7 @@ const fs = require('fs'); const remapIstanbul = require('remap-istanbul'); const istanbul = require('istanbul'); const glob = require('glob'); +const os = require('os'); /** * Hygiene works by creating cascading subsets of all our files and @@ -68,7 +69,8 @@ const copyrightHeader = [ '// Licensed under the MIT License.', '', '\'use strict\';' -].join('\n'); +]; +const copyrightHeaders = [copyrightHeader.join('\n'), copyrightHeader.join('\r\n')]; gulp.task('hygiene', () => run({ mode: 'all', skipFormatCheck: true, skipIndentationCheck: true })); @@ -146,10 +148,13 @@ const hygiene = (options) => { console.log(colors.blue('Hygiene started.')); const copyrights = es.through(function (file) { - if (addedFiles.indexOf(file.path) !== -1 && file.contents.toString('utf8').indexOf(copyrightHeader) !== 0) { - // Use tslint format. - console.error(`ERROR: (copyright) ${file.relative}[1,1]: Missing or bad copyright statement`); - errorCount++; + if (addedFiles.indexOf(file.path) !== -1) { + const contents = file.contents.toString('utf8'); + if (!copyrightHeaders.some(header => contents.indexOf(header) === 0)) { + // Use tslint format. + console.error(`ERROR: (copyright) ${file.relative}[1,1]: Missing or bad copyright statement`); + errorCount++; + } } this.emit('data', file); From 11322f38040f8eed0dae71895c04b3fd1fd7f72f Mon Sep 17 00:00:00 2001 From: Don Jayamanne Date: Thu, 22 Mar 2018 14:16:28 -0700 Subject: [PATCH 3/4] :white_check_mark: tests --- src/client/linters/prospector.ts | 2 +- src/test/linters/lint.args.test.ts | 155 +++++++++++++++++++++++++++++ 2 files changed, 156 insertions(+), 1 deletion(-) create mode 100644 src/test/linters/lint.args.test.ts diff --git a/src/client/linters/prospector.ts b/src/client/linters/prospector.ts index 5642c5433848..8bbef82c46a5 100644 --- a/src/client/linters/prospector.ts +++ b/src/client/linters/prospector.ts @@ -28,7 +28,7 @@ export class Prospector extends BaseLinter { } protected async runLinter(document: TextDocument, cancellation: CancellationToken): Promise { - return this.run(['--absolute-paths', '--output-format=json', document.uri.fsPath], document, cancellation); + return this.run(['--absolute-paths', '--output-format=json', document.uri.fsPath.fileToCommandArgument()], document, cancellation); } protected async parseMessages(output: string, document: TextDocument, token: CancellationToken, regEx: string) { let parsedData: IProspectorResponse; diff --git a/src/test/linters/lint.args.test.ts b/src/test/linters/lint.args.test.ts new file mode 100644 index 000000000000..259f87e38ef7 --- /dev/null +++ b/src/test/linters/lint.args.test.ts @@ -0,0 +1,155 @@ +// Copyright (c) Microsoft Corporation. All rights reserved. +// Licensed under the MIT License. + +'use strict'; + +// tslint:disable:no-any + +import { expect } from 'chai'; +import { Container } from 'inversify'; +import * as path from 'path'; +import * as TypeMoq from 'typemoq'; +import { CancellationTokenSource, OutputChannel, TextDocument, Uri } from 'vscode'; +import { IDocumentManager, IWorkspaceService } from '../../client/common/application/types'; +import '../../client/common/extensions'; +import { IFileSystem, IPlatformService } from '../../client/common/platform/types'; +import { IConfigurationService, IInstaller, ILintingSettings, ILogger, IOutputChannel, IPythonSettings } from '../../client/common/types'; +import { IInterpreterService } from '../../client/interpreter/contracts'; +import { ServiceContainer } from '../../client/ioc/container'; +import { ServiceManager } from '../../client/ioc/serviceManager'; +import { BaseLinter } from '../../client/linters/baseLinter'; +import { Flake8 } from '../../client/linters/flake8'; +import { LinterManager } from '../../client/linters/linterManager'; +import { MyPy } from '../../client/linters/mypy'; +import { Pep8 } from '../../client/linters/pep8'; +import { Prospector } from '../../client/linters/prospector'; +import { PyDocStyle } from '../../client/linters/pydocstyle'; +import { PyLama } from '../../client/linters/pylama'; +import { Pylint } from '../../client/linters/pylint'; +import { ILinterManager, ILintingEngine } from '../../client/linters/types'; +import { initialize } from '../initialize'; + +// tslint:disable-next-line:max-func-body-length +suite('Linting - Arguments', () => { + let interpreterService: TypeMoq.IMock; + let engine: TypeMoq.IMock; + let configService: TypeMoq.IMock; + let docManager: TypeMoq.IMock; + let settings: TypeMoq.IMock; + let lm: ILinterManager; + let serviceContainer: ServiceContainer; + let document: TypeMoq.IMock; + let outputChannel: TypeMoq.IMock; + let workspaceService: TypeMoq.IMock; + const cancellationToken = new CancellationTokenSource().token; + + suiteSetup(initialize); + setup(async () => { + const cont = new Container(); + const serviceManager = new ServiceManager(cont); + + serviceContainer = new ServiceContainer(cont); + outputChannel = TypeMoq.Mock.ofType(); + + const fs = TypeMoq.Mock.ofType(); + fs.setup(x => x.fileExistsAsync(TypeMoq.It.isAny())).returns(() => new Promise((resolve, reject) => resolve(true))); + fs.setup(x => x.arePathsSame(TypeMoq.It.isAnyString(), TypeMoq.It.isAnyString())).returns(() => true); + serviceManager.addSingletonInstance(IFileSystem, fs.object); + + serviceManager.addSingletonInstance(IOutputChannel, outputChannel.object); + + interpreterService = TypeMoq.Mock.ofType(); + serviceManager.addSingletonInstance(IInterpreterService, interpreterService.object); + + engine = TypeMoq.Mock.ofType(); + serviceManager.addSingletonInstance(ILintingEngine, engine.object); + + docManager = TypeMoq.Mock.ofType(); + serviceManager.addSingletonInstance(IDocumentManager, docManager.object); + + const lintSettings = TypeMoq.Mock.ofType(); + lintSettings.setup(x => x.enabled).returns(() => true); + lintSettings.setup(x => x.lintOnSave).returns(() => true); + + settings = TypeMoq.Mock.ofType(); + settings.setup(x => x.linting).returns(() => lintSettings.object); + + configService = TypeMoq.Mock.ofType(); + configService.setup(x => x.getSettings(TypeMoq.It.isAny())).returns(() => settings.object); + serviceManager.addSingletonInstance(IConfigurationService, configService.object); + + workspaceService = TypeMoq.Mock.ofType(); + serviceManager.addSingletonInstance(IWorkspaceService, workspaceService.object); + + const logger = TypeMoq.Mock.ofType(); + serviceManager.addSingletonInstance(ILogger, logger.object); + + const installer = TypeMoq.Mock.ofType(); + serviceManager.addSingletonInstance(IInstaller, installer.object); + + const platformService = TypeMoq.Mock.ofType(); + serviceManager.addSingletonInstance(IPlatformService, platformService.object); + + lm = new LinterManager(serviceContainer); + serviceManager.addSingletonInstance(ILinterManager, lm); + document = TypeMoq.Mock.ofType(); + }); + + async function testLinter(linter: BaseLinter, fileUri: Uri, expectedArgs: string[]) { + document.setup(d => d.uri).returns(() => fileUri); + + let invoked = false; + (linter as any).run = (args, doc, token) => { + expect(args).to.deep.equal(expectedArgs); + invoked = true; + return Promise.resolve([]); + }; + await linter.lint(document.object, cancellationToken); + expect(invoked).to.be.equal(true, 'method not invoked'); + } + [Uri.file(path.join('users', 'development path to', 'one.py')), Uri.file(path.join('users', 'development', 'one.py'))].forEach(fileUri => { + test(`Flake8 (${fileUri.fsPath.indexOf(' ') > 0 ? 'with spaces' : 'without spaces'})`, async () => { + const linter = new Flake8(outputChannel.object, serviceContainer); + const expectedArgs = ['--format=%(row)d,%(col)d,%(code).1s,%(code)s:%(text)s', fileUri.fsPath.fileToCommandArgument()]; + await testLinter(linter, fileUri, expectedArgs); + }); + test(`Pep8 (${fileUri.fsPath.indexOf(' ') > 0 ? 'with spaces' : 'without spaces'})`, async () => { + const linter = new Pep8(outputChannel.object, serviceContainer); + const expectedArgs = ['--format=%(row)d,%(col)d,%(code).1s,%(code)s:%(text)s', fileUri.fsPath.fileToCommandArgument()]; + await testLinter(linter, fileUri, expectedArgs); + }); + test(`Prospector (${fileUri.fsPath.indexOf(' ') > 0 ? 'with spaces' : 'without spaces'})`, async () => { + const linter = new Prospector(outputChannel.object, serviceContainer); + const expectedArgs = ['--absolute-paths', '--output-format=json', fileUri.fsPath.fileToCommandArgument()]; + await testLinter(linter, fileUri, expectedArgs); + }); + test(`Pylama (${fileUri.fsPath.indexOf(' ') > 0 ? 'with spaces' : 'without spaces'})`, async () => { + const linter = new PyLama(outputChannel.object, serviceContainer); + const expectedArgs = ['--format=parsable', fileUri.fsPath.fileToCommandArgument()]; + await testLinter(linter, fileUri, expectedArgs); + }); + test(`MyPy (${fileUri.fsPath.indexOf(' ') > 0 ? 'with spaces' : 'without spaces'})`, async () => { + const linter = new MyPy(outputChannel.object, serviceContainer); + const expectedArgs = [fileUri.fsPath.fileToCommandArgument()]; + await testLinter(linter, fileUri, expectedArgs); + }); + test(`Pydocstyle (${fileUri.fsPath.indexOf(' ') > 0 ? 'with spaces' : 'without spaces'})`, async () => { + const linter = new PyDocStyle(outputChannel.object, serviceContainer); + const expectedArgs = [fileUri.fsPath.fileToCommandArgument()]; + await testLinter(linter, fileUri, expectedArgs); + }); + test(`Pylint (${fileUri.fsPath.indexOf(' ') > 0 ? 'with spaces' : 'without spaces'})`, async () => { + const linter = new Pylint(outputChannel.object, serviceContainer); + document.setup(d => d.uri).returns(() => fileUri); + + let invoked = false; + (linter as any).run = (args, doc, token) => { + expect(args[args.length - 1]).to.equal(fileUri.fsPath.fileToCommandArgument()); + invoked = true; + return Promise.resolve([]); + }; + await linter.lint(document.object, cancellationToken); + expect(invoked).to.be.equal(true, 'method not invoked'); + }); + }); +}); From 3edf442a24191d26526d088c19cedb0cf300c6da Mon Sep 17 00:00:00 2001 From: Don Jayamanne Date: Thu, 22 Mar 2018 14:21:46 -0700 Subject: [PATCH 4/4] :memo: news entry --- news/2 Fixes/199.md | 1 + 1 file changed, 1 insertion(+) create mode 100644 news/2 Fixes/199.md diff --git a/news/2 Fixes/199.md b/news/2 Fixes/199.md new file mode 100644 index 000000000000..94f155e516b8 --- /dev/null +++ b/news/2 Fixes/199.md @@ -0,0 +1 @@ +Ensures file paths are properly encoded when passing them as arguments to linters.