From 79d73b064cfc9077597469aa4874c4d204499814 Mon Sep 17 00:00:00 2001 From: Xiao Liang Date: Thu, 30 May 2019 05:51:23 +0800 Subject: [PATCH 1/5] fix(client/linters/mypy): call mypy incorrectly It's supposed to call `mypy` in the root package directory with the relative path to the python file being checked. This commit enforces this behavior. --- src/client/linters/mypy.ts | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/client/linters/mypy.ts b/src/client/linters/mypy.ts index 7c8559881a42..30f94eb10c93 100644 --- a/src/client/linters/mypy.ts +++ b/src/client/linters/mypy.ts @@ -4,6 +4,7 @@ import { Product } from '../common/types'; import { IServiceContainer } from '../ioc/types'; import { BaseLinter } from './baseLinter'; import { ILintMessage } from './types'; +import * as path from 'path'; export const REGEX = '(?[^:]+):(?\\d+)(:(?\\d+))?: (?\\w+): (?.*)\\r?(\\n|$)'; @@ -13,7 +14,9 @@ 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 cwd = this.getWorkspaceRootPath(document); + const relativePath = path.relative(cwd,document.uri.fsPath); + const messages = await this.runrelativePath document, cancellation, REGEX); messages.forEach(msg => { msg.severity = this.parseMessagesSeverity(msg.type, this.pythonSettings.linting.mypyCategorySeverity); msg.code = msg.type; From bdf17aa9a1de2b24d2dc9e887e84dd641f30b468 Mon Sep 17 00:00:00 2001 From: Xiao Liang Date: Fri, 31 May 2019 04:07:42 +0800 Subject: [PATCH 2/5] fix(client/linters/mypy): syntax error --- src/client/linters/mypy.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/client/linters/mypy.ts b/src/client/linters/mypy.ts index 30f94eb10c93..933e67caad46 100644 --- a/src/client/linters/mypy.ts +++ b/src/client/linters/mypy.ts @@ -15,8 +15,8 @@ export class MyPy extends BaseLinter { protected async runLinter(document: TextDocument, cancellation: CancellationToken): Promise { const cwd = this.getWorkspaceRootPath(document); - const relativePath = path.relative(cwd,document.uri.fsPath); - const messages = await this.runrelativePath document, cancellation, REGEX); + const relativePath = path.relative(cwd, document.uri.fsPath); + const messages = await this.run([relativePath], document, cancellation, REGEX); messages.forEach(msg => { msg.severity = this.parseMessagesSeverity(msg.type, this.pythonSettings.linting.mypyCategorySeverity); msg.code = msg.type; From 3c1a4f39497820b0d7fa29afa5783d2471e84897 Mon Sep 17 00:00:00 2001 From: Karthik Nadig Date: Fri, 8 Nov 2019 16:06:02 -0800 Subject: [PATCH 3/5] Fixes and tests --- src/client/linters/mypy.ts | 2 +- src/test/linters/mypy.unit.test.ts | 87 +++++++++++++++++++++++++++--- 2 files changed, 82 insertions(+), 7 deletions(-) diff --git a/src/client/linters/mypy.ts b/src/client/linters/mypy.ts index 933e67caad46..fb5de8442f4f 100644 --- a/src/client/linters/mypy.ts +++ b/src/client/linters/mypy.ts @@ -1,10 +1,10 @@ +import * as path from 'path'; import { CancellationToken, OutputChannel, TextDocument } from 'vscode'; import '../common/extensions'; import { Product } from '../common/types'; import { IServiceContainer } from '../ioc/types'; import { BaseLinter } from './baseLinter'; import { ILintMessage } from './types'; -import * as path from 'path'; export const REGEX = '(?[^:]+):(?\\d+)(:(?\\d+))?: (?\\w+): (?.*)\\r?(\\n|$)'; diff --git a/src/test/linters/mypy.unit.test.ts b/src/test/linters/mypy.unit.test.ts index 1f6e36146c1b..9c7daeb160f1 100644 --- a/src/test/linters/mypy.unit.test.ts +++ b/src/test/linters/mypy.unit.test.ts @@ -6,12 +6,20 @@ // tslint:disable:no-object-literal-type-assertion import { expect } from 'chai'; +import * as path from 'path'; +import * as sinon from 'sinon'; +import { anything, instance, mock, when } from 'ts-mockito'; +import { CancellationToken, CancellationTokenSource, TextDocument, Uri } from 'vscode'; +import { Product } from '../../client/common/types'; +import { ServiceContainer } from '../../client/ioc/container'; import { parseLine } from '../../client/linters/baseLinter'; -import { REGEX } from '../../client/linters/mypy'; -import { ILintMessage } from '../../client/linters/types'; +import { LinterManager } from '../../client/linters/linterManager'; +import { MyPy, REGEX } from '../../client/linters/mypy'; +import { ILinterManager, ILintMessage, LintMessageSeverity } from '../../client/linters/types'; +import { MockOutputChannel } from '../mockClasses'; // This following is a real-world example. See gh=2380. -// tslint:disable-next-line:no-multiline-string +// tslint:disable:no-multiline-string no-any max-func-body-length const output = ` provider.pyi:10: error: Incompatible types in assignment (expression has type "str", variable has type "int") provider.pyi:11: error: Name 'not_declared_var' is not defined @@ -29,7 +37,7 @@ suite('Linting - MyPy', () => { line: 10, type: 'error', provider: 'mypy' - } as ILintMessage], + } as ILintMessage], [lines[2], { code: undefined, message: 'Name \'not_declared_var\' is not defined', @@ -37,7 +45,7 @@ suite('Linting - MyPy', () => { line: 11, type: 'error', provider: 'mypy' - } as ILintMessage], + } as ILintMessage], [lines[3], { code: undefined, message: 'Expression has type "Any"', @@ -45,7 +53,7 @@ suite('Linting - MyPy', () => { line: 12, type: 'error', provider: 'mypy' - } as ILintMessage] + } as ILintMessage] ]; for (const [line, expected] of tests) { const msg = parseLine(line, REGEX, 'mypy'); @@ -53,4 +61,71 @@ suite('Linting - MyPy', () => { expect(msg).to.deep.equal(expected); } }); + class TestMyPyLinter extends MyPy { + // tslint:disable: no-unnecessary-override + public async runLinter(document: TextDocument, cancellation: CancellationToken): Promise { + return super.runLinter(document, cancellation); + } + public getWorkspaceRootPath(document: TextDocument): string { + return super.getWorkspaceRootPath(document); + } + public async run(args: string[], document: TextDocument, cancellation: CancellationToken, regEx: string = REGEX): Promise { + return super.run(args, document, cancellation, regEx); + } + public parseMessagesSeverity(error: string, severity: any): LintMessageSeverity { + return super.parseMessagesSeverity(error, severity); + } + } + suite('Test Linter', () => { + let linter: TestMyPyLinter; + let getWorkspaceRootPathStub: sinon.SinonStub<[TextDocument], string>; + let runStub: sinon.SinonStub<[string[], TextDocument, CancellationToken, (string | undefined)?], Promise>; + const token = new CancellationTokenSource().token; + teardown(() => sinon.restore()); + setup(() => { + const linterManager = mock(LinterManager); + when(linterManager.getLinterInfo(anything())).thenReturn({ product: Product.mypy } as any); + const serviceContainer = mock(ServiceContainer); + when(serviceContainer.get(ILinterManager)).thenReturn(instance(linterManager)); + getWorkspaceRootPathStub = sinon.stub(TestMyPyLinter.prototype, 'getWorkspaceRootPath'); + runStub = sinon.stub(TestMyPyLinter.prototype, 'run'); + linter = new TestMyPyLinter(instance(mock(MockOutputChannel)), instance(serviceContainer)); + }); + + test('Get cwd based on document', async () => { + const fileUri = Uri.file(path.join('a', 'b', 'c', 'd', 'e', 'filename.py')); + const cwd = path.join('a', 'b', 'c'); + const doc = { uri: fileUri } as any as TextDocument; + getWorkspaceRootPathStub.callsFake(() => cwd); + runStub.callsFake(() => Promise.resolve([])); + + await linter.runLinter(doc, token); + + expect(getWorkspaceRootPathStub.callCount).to.equal(1); + expect(getWorkspaceRootPathStub.args[0]).to.deep.equal([doc]); + }); + test('Pass relative path of document to linter', async () => { + const fileUri = Uri.file(path.join('a', 'b', 'c', 'd', 'e', 'filename.py')); + const cwd = path.join('a', 'b', 'c'); + const doc = { uri: fileUri } as any as TextDocument; + getWorkspaceRootPathStub.callsFake(() => cwd); + runStub.callsFake(() => Promise.resolve([])); + + await linter.runLinter(doc, token); + + expect(runStub.callCount).to.equal(1); + expect(runStub.args[0]).to.deep.equal([[path.relative(cwd, fileUri.fsPath)], doc, token, REGEX]); + }); + test('Return empty messages', async () => { + const fileUri = Uri.file(path.join('a', 'b', 'c', 'd', 'e', 'filename.py')); + const cwd = path.join('a', 'b', 'c'); + const doc = { uri: fileUri } as any as TextDocument; + getWorkspaceRootPathStub.callsFake(() => cwd); + runStub.callsFake(() => Promise.resolve([])); + + const messages = await linter.runLinter(doc, token); + + expect(messages).to.be.deep.equal([]); + }); + }); }); From d326acca1c659a5e244444f65af05c5d3c017c37 Mon Sep 17 00:00:00 2001 From: Karthik Nadig Date: Fri, 8 Nov 2019 16:11:01 -0800 Subject: [PATCH 4/5] Add news item. --- news/2 Fixes/5326.md | 2 ++ 1 file changed, 2 insertions(+) create mode 100644 news/2 Fixes/5326.md diff --git a/news/2 Fixes/5326.md b/news/2 Fixes/5326.md new file mode 100644 index 000000000000..d23738474186 --- /dev/null +++ b/news/2 Fixes/5326.md @@ -0,0 +1,2 @@ +Use relative paths when invoking mypy. +(thanks to [yxliang01](https://github.com/yxliang01)) From a3088f53943ab91b75e5aee19bb15fc1fb70fe5c Mon Sep 17 00:00:00 2001 From: Karthik Nadig Date: Fri, 8 Nov 2019 16:16:15 -0800 Subject: [PATCH 5/5] House keeping on tests --- src/test/linters/mypy.unit.test.ts | 90 +++++++++++++++--------------- 1 file changed, 46 insertions(+), 44 deletions(-) diff --git a/src/test/linters/mypy.unit.test.ts b/src/test/linters/mypy.unit.test.ts index 9c7daeb160f1..2dd0c01ba1a5 100644 --- a/src/test/linters/mypy.unit.test.ts +++ b/src/test/linters/mypy.unit.test.ts @@ -61,6 +61,9 @@ suite('Linting - MyPy', () => { expect(msg).to.deep.equal(expected); } }); +}); + +suite('Test Linter', () => { class TestMyPyLinter extends MyPy { // tslint:disable: no-unnecessary-override public async runLinter(document: TextDocument, cancellation: CancellationToken): Promise { @@ -76,56 +79,55 @@ suite('Linting - MyPy', () => { return super.parseMessagesSeverity(error, severity); } } - suite('Test Linter', () => { - let linter: TestMyPyLinter; - let getWorkspaceRootPathStub: sinon.SinonStub<[TextDocument], string>; - let runStub: sinon.SinonStub<[string[], TextDocument, CancellationToken, (string | undefined)?], Promise>; - const token = new CancellationTokenSource().token; - teardown(() => sinon.restore()); - setup(() => { - const linterManager = mock(LinterManager); - when(linterManager.getLinterInfo(anything())).thenReturn({ product: Product.mypy } as any); - const serviceContainer = mock(ServiceContainer); - when(serviceContainer.get(ILinterManager)).thenReturn(instance(linterManager)); - getWorkspaceRootPathStub = sinon.stub(TestMyPyLinter.prototype, 'getWorkspaceRootPath'); - runStub = sinon.stub(TestMyPyLinter.prototype, 'run'); - linter = new TestMyPyLinter(instance(mock(MockOutputChannel)), instance(serviceContainer)); - }); - test('Get cwd based on document', async () => { - const fileUri = Uri.file(path.join('a', 'b', 'c', 'd', 'e', 'filename.py')); - const cwd = path.join('a', 'b', 'c'); - const doc = { uri: fileUri } as any as TextDocument; - getWorkspaceRootPathStub.callsFake(() => cwd); - runStub.callsFake(() => Promise.resolve([])); + let linter: TestMyPyLinter; + let getWorkspaceRootPathStub: sinon.SinonStub<[TextDocument], string>; + let runStub: sinon.SinonStub<[string[], TextDocument, CancellationToken, (string | undefined)?], Promise>; + const token = new CancellationTokenSource().token; + teardown(() => sinon.restore()); + setup(() => { + const linterManager = mock(LinterManager); + when(linterManager.getLinterInfo(anything())).thenReturn({ product: Product.mypy } as any); + const serviceContainer = mock(ServiceContainer); + when(serviceContainer.get(ILinterManager)).thenReturn(instance(linterManager)); + getWorkspaceRootPathStub = sinon.stub(TestMyPyLinter.prototype, 'getWorkspaceRootPath'); + runStub = sinon.stub(TestMyPyLinter.prototype, 'run'); + linter = new TestMyPyLinter(instance(mock(MockOutputChannel)), instance(serviceContainer)); + }); + + test('Get cwd based on document', async () => { + const fileUri = Uri.file(path.join('a', 'b', 'c', 'd', 'e', 'filename.py')); + const cwd = path.join('a', 'b', 'c'); + const doc = { uri: fileUri } as any as TextDocument; + getWorkspaceRootPathStub.callsFake(() => cwd); + runStub.callsFake(() => Promise.resolve([])); - await linter.runLinter(doc, token); + await linter.runLinter(doc, token); - expect(getWorkspaceRootPathStub.callCount).to.equal(1); - expect(getWorkspaceRootPathStub.args[0]).to.deep.equal([doc]); - }); - test('Pass relative path of document to linter', async () => { - const fileUri = Uri.file(path.join('a', 'b', 'c', 'd', 'e', 'filename.py')); - const cwd = path.join('a', 'b', 'c'); - const doc = { uri: fileUri } as any as TextDocument; - getWorkspaceRootPathStub.callsFake(() => cwd); - runStub.callsFake(() => Promise.resolve([])); + expect(getWorkspaceRootPathStub.callCount).to.equal(1); + expect(getWorkspaceRootPathStub.args[0]).to.deep.equal([doc]); + }); + test('Pass relative path of document to linter', async () => { + const fileUri = Uri.file(path.join('a', 'b', 'c', 'd', 'e', 'filename.py')); + const cwd = path.join('a', 'b', 'c'); + const doc = { uri: fileUri } as any as TextDocument; + getWorkspaceRootPathStub.callsFake(() => cwd); + runStub.callsFake(() => Promise.resolve([])); - await linter.runLinter(doc, token); + await linter.runLinter(doc, token); - expect(runStub.callCount).to.equal(1); - expect(runStub.args[0]).to.deep.equal([[path.relative(cwd, fileUri.fsPath)], doc, token, REGEX]); - }); - test('Return empty messages', async () => { - const fileUri = Uri.file(path.join('a', 'b', 'c', 'd', 'e', 'filename.py')); - const cwd = path.join('a', 'b', 'c'); - const doc = { uri: fileUri } as any as TextDocument; - getWorkspaceRootPathStub.callsFake(() => cwd); - runStub.callsFake(() => Promise.resolve([])); + expect(runStub.callCount).to.equal(1); + expect(runStub.args[0]).to.deep.equal([[path.relative(cwd, fileUri.fsPath)], doc, token, REGEX]); + }); + test('Return empty messages', async () => { + const fileUri = Uri.file(path.join('a', 'b', 'c', 'd', 'e', 'filename.py')); + const cwd = path.join('a', 'b', 'c'); + const doc = { uri: fileUri } as any as TextDocument; + getWorkspaceRootPathStub.callsFake(() => cwd); + runStub.callsFake(() => Promise.resolve([])); - const messages = await linter.runLinter(doc, token); + const messages = await linter.runLinter(doc, token); - expect(messages).to.be.deep.equal([]); - }); + expect(messages).to.be.deep.equal([]); }); });