Ignore mypy lint errors for other files#15266

sbdchd · 2021-01-29T00:31:52Z

mypy sometimes returns errors for files that aren't the same as the file
it was called against.

This PR adds some filtering based on the current file name.

Test file structure:

❯ tree foo
foo
├── __init__.py
└── bar.py

# __init__.py
def main() -> None:
    return None
    print("something")

# bar.py
def foo() -> None:
    x: int = "foo"

Before:

After:

fixes #10190
related #15216

mypy sometimes returns errors for files that aren't the same as the file it was called against. This PR adds some filtering based on the current file name. fixes microsoft#10190

sbdchd · 2021-01-29T00:34:52Z

src/client/linters/mypy.ts

@@ -14,10 +15,19 @@ export class MyPy extends BaseLinter {

    protected async runLinter(document: TextDocument, cancellation: CancellationToken): Promise<ILintMessage[]> {
        const messages = await this.run([document.uri.fsPath], document, cancellation, REGEX);


Another option: generate the regex used to extract the message info at runtime with the current file name substituted instead of the file capture group, causing the messages for the other files to be ignored

sbdchd · 2021-01-29T00:46:22Z

src/client/linters/mypy.ts


    protected async runLinter(document: TextDocument, cancellation: CancellationToken): Promise<ILintMessage[]> {
-        const messages = await this.run([document.uri.fsPath], document, cancellation, REGEX);
+        const fileName = document.uri.fsPath.slice(this.getWorkspaceRootPath(document).length + 1);


original approach was to modify the ILintMessage to include an optional file property and then filter based off that here

regex approach seems less invasive

also wondering the best way to write a test for this

karrtikr

Thanks for the PR! Almost there,

src/client/linters/mypy.ts

src/test/linters/mypy.unit.test.ts

sbdchd · 2021-02-25T03:57:59Z

src/client/linters/mypy.ts


    protected async runLinter(document: TextDocument, cancellation: CancellationToken): Promise<ILintMessage[]> {
-        const messages = await this.run([document.uri.fsPath], document, cancellation, REGEX);
+        const relativeFilePath = document.uri.fsPath.slice(this.getWorkspaceRootPath(document).length + 1);


document.uri.fsPath gives us "/Users/foo/workspace-project/foo/bar.py" but we want the path relative to the workspace: foo/bar.py

``` ##########Linting Output - mypy########## foo/__init__.py:4:5: error: Statement is unreachable [unreachable] foo/bar.py:2:14: error: Incompatible types in assignment (expression has type "str", variable has type "int") [assignment] Found 2 errors in 2 files (checked 1 source file) ```

codecov-io · 2021-02-25T04:08:26Z

Codecov Report

Merging #15266 (434eded) into main (790c909) will increase coverage by 0%.
The diff coverage is 71%.

@@           Coverage Diff           @@
##            main   #15266    +/-   ##
=======================================
  Coverage     65%      65%            
=======================================
  Files        558      561     +3     
  Lines      26646    26982   +336     
  Branches    3840     3914    +74     
=======================================
+ Hits       17322    17562   +240     
- Misses      8617     8687    +70     
- Partials     707      733    +26

Impacted Files	Coverage Δ
src/client/linters/mypy.ts	`41% <50%> (+2%)`	⬆️
...nments/discovery/locators/services/condaService.ts	`60% <50%> (-25%)`	⬇️
...ent/interpreter/autoSelection/rules/winRegistry.ts	`93% <100%> (+<1%)`	⬆️
src/client/pythonEnvironments/legacyIOC.ts	`37% <100%> (+2%)`	⬆️
src/client/common/utils/filesystem.ts	`27% <0%> (-17%)`	⬇️
...sting/common/services/unitTestDiagnosticService.ts	`80% <0%> (-16%)`	⬇️
...rc/client/pythonEnvironments/common/commonUtils.ts	`63% <0%> (-12%)`	⬇️
...nments/base/locators/lowLevel/fsWatchingLocator.ts	`77% <0%> (-8%)`	⬇️
src/client/testing/common/types.ts	`93% <0%> (-7%)`	⬇️
... and 118 more

karrtikr · 2021-02-25T20:08:39Z

Thanks for the changes, I'll have a look as soon as possible. FYI #15505 could affect this PR.

karrtikr · 2021-03-05T18:30:15Z

@sbdchd #15505 just got merged, but it may affect this PR. Can you please rebase off main. (Use git rebase main, and then git push -f to force push the changes here)

karrtikr

LGTM once the offset is adjusted via rebase

karrtikr · 2021-03-05T18:54:39Z

src/test/linters/mypy.unit.test.ts

+            [lines[2], undefined],
+        ];
+        for (const [line, expected] of tests) {
+            const msg = parseLine(line, getRegex('foo/bar.py'), LinterId.MyPy);


Column offset should be passed as 1.

karrtikr · 2021-03-09T18:05:03Z

@sbdchd The tests seem to be failing, can you please rebase again? The fixes may have landed on main branch.

sbdchd · 2021-03-09T22:53:28Z

I think I fixed the tests, I'm having trouble getting the test suite to fail locally

If I throw an exception or expect(true).to.eql(false); in a test, they are still passing, not sure what's up

❯ npm run test:unittests -- --grep='Linting - MyPy'                                                                                                     3.05s

> python@2021.4.0-dev test:unittests /Users/badger/Downloads/vscode-python
> mocha --config ./build/.mocha.unittests.js.json "--grep=Linting - MyPy"



  Linting - MyPy
  ✓ regex
  ✓ regex excludes unexpected files
  ✓ getRegex escapes filename correctly


  3 passing (36ms)

karrtikr · 2021-03-09T22:58:25Z

Interesting, we generally use expect(true).to.equal(false) syntax instead, I'm not sure about eql. Anyways, if the tests pass on CI this should be good to go.

Ignore mypy lint errors for other files

b28a68c

mypy sometimes returns errors for files that aren't the same as the file it was called against. This PR adds some filtering based on the current file name. fixes microsoft#10190

github-actions bot assigned karrtikr Jan 29, 2021

github-actions bot requested review from karrtikr and kimadeline January 29, 2021 00:32

sbdchd commented Jan 29, 2021

View reviewed changes

use regex instead

9c999bc

sbdchd commented Jan 29, 2021

View reviewed changes

add news

a840443

karrtikr reviewed Feb 24, 2021

View reviewed changes

src/client/linters/mypy.ts Outdated Show resolved Hide resolved

src/test/linters/mypy.unit.test.ts Outdated Show resolved Hide resolved

src/test/linters/mypy.unit.test.ts Show resolved Hide resolved

karrtikr mentioned this pull request Feb 24, 2021

Expose a setting to allow a user modify the working directory of linters #15171

Merged

6 tasks

code review fixes

434eded

sbdchd commented Feb 25, 2021

View reviewed changes

karrtikr self-requested a review February 25, 2021 20:07

karrtikr approved these changes Mar 5, 2021

View reviewed changes

sbdchd added 2 commits March 5, 2021 17:44

Merge remote-tracking branch 'origin/main' into main

0fa7d6c

fix offset

9c7b951

kimadeline approved these changes Mar 9, 2021

View reviewed changes

sbdchd added 2 commits March 9, 2021 17:46

Merge remote-tracking branch 'origin/main' into main

a8b11c1

fix test?

75830b2

karrtikr merged commit 3d4c382 into microsoft:main Mar 9, 2021

		@@ -14,10 +15,19 @@ export class MyPy extends BaseLinter {

		protected async runLinter(document: TextDocument, cancellation: CancellationToken): Promise<ILintMessage[]> {
		const messages = await this.run([document.uri.fsPath], document, cancellation, REGEX);

Conversation

sbdchd commented Jan 29, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sbdchd Jan 29, 2021

Choose a reason for hiding this comment

Uh oh!

sbdchd Jan 29, 2021

Choose a reason for hiding this comment

Uh oh!

sbdchd Jan 29, 2021

Choose a reason for hiding this comment

Uh oh!

karrtikr left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sbdchd Feb 25, 2021

Choose a reason for hiding this comment

Uh oh!

codecov-io commented Feb 25, 2021

Codecov Report

Uh oh!

karrtikr commented Feb 25, 2021

Uh oh!

karrtikr commented Mar 5, 2021

Uh oh!

karrtikr left a comment

Choose a reason for hiding this comment

Uh oh!

karrtikr Mar 5, 2021

Choose a reason for hiding this comment

Uh oh!

karrtikr commented Mar 9, 2021

Uh oh!

sbdchd commented Mar 9, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

karrtikr commented Mar 9, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

sbdchd commented Jan 29, 2021 •

edited

Loading

sbdchd commented Mar 9, 2021 •

edited

Loading