Skip to content

Fix MyPy linter message REGEX to handle any python extension#2402

Merged
DonJayamanne merged 2 commits intomicrosoft:masterfrom
AllanDaemon:mypy_fix
Sep 25, 2018
Merged

Fix MyPy linter message REGEX to handle any python extension#2402
DonJayamanne merged 2 commits intomicrosoft:masterfrom
AllanDaemon:mypy_fix

Conversation

@AllanDaemon
Copy link

@AllanDaemon AllanDaemon commented Aug 16, 2018

Fixes #2380
The regex expression to match MyPy linter messages expects that the file name has a .py extension, that isn't always the case. E.g., .pyi files for describing interfaces.

  • Title summarizes what is changing
  • Includes a news entry file (remember to thank yourself!)
  • Unit tests & code coverage are not adversely affected (within reason)
  • Works on all actively maintained versions of Python (e.g. Python 2.7 & the latest Python 3 release)
  • Works on Windows 10, macOS, and Linux (e.g. considered file system case-sensitivity)
  • Dependencies are pinned (e.g. "1.2.3", not "^1.2.3")
  • package-lock.json has been regenerated if dependencies have changed

@msftclas
Copy link

msftclas commented Aug 16, 2018

CLA assistant check
All CLA requirements met.

@codecov
Copy link

codecov bot commented Aug 16, 2018

Codecov Report

Merging #2402 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2402      +/-   ##
==========================================
+ Coverage   75.38%   75.39%   +<.01%     
==========================================
  Files         309      309              
  Lines       14331    14331              
  Branches     2537     2537              
==========================================
+ Hits        10804    10805       +1     
+ Misses       3518     3517       -1     
  Partials        9        9
Flag Coverage Δ
#MacOS 74.18% <100%> (-0.03%) ⬇️
#Windows 74.33% <100%> (+0.03%) ⬆️
Impacted Files Coverage Δ
src/client/linters/mypy.ts 84.61% <100%> (ø) ⬆️
src/client/linters/lintingEngine.ts 92.03% <0%> (+0.88%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6707eaa...eca11c6. Read the comment docs.

Copy link

@d3r3kk d3r3kk left a comment

Choose a reason for hiding this comment

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

First, thank you for taking the time to submit this PR.

However, I'm not going to approve this as I want @brettcannon or @ericsnowcurrently's eyes on it first.

...handle any python [file] extension...

I'm not sure allowing any file extension at all is a great idea here. I think I could get behind explicit python file extensions, but to simply guess and assume that only python files will be picked up seems a bit too loose perhaps?
I assume that mypy will only pick up relevant Python files, can we always rely on that being true?

@d3r3kk
Copy link

d3r3kk commented Aug 16, 2018

(Also, before I would accept this I'd want to see some tests to ensure that it is exhibiting the behaviour you desire and not any undesired behaviours as well)

@brettcannon
Copy link
Member

Maybe @DonJayamanne remembers a reason to explicitly match a .py file? If there is then I would change that the regex to be \\.pyi?.

@brettcannon
Copy link
Member

And can I ask what the use-case is? Since .pyi files have no code to really execute I'm not sure what kind of errors are you expecting to see?

@AllanDaemon
Copy link
Author

AllanDaemon commented Aug 20, 2018

It already runs MyPy and PyLinter with the .pyi files, as you can see in the output log of #2380. This modification is just for the RegEx to filter the MyPy output to catch the messages that should be sent to to the problems tab.

Also, the pylint messages for .pyi files are already working, and they don't even have the filename in them (the terminal output parsed by the extension).

As for the use, in my specific case, I do them to write the API "interface", to model it before writing the code, using typing hints to check any mistake taken in this part. So MyPy should never run code in fact, just check the typing soundness of the interfaces (methods signatures, class members, etc).

@DonJayamanne DonJayamanne merged commit bfcd225 into microsoft:master Sep 25, 2018
@lock lock bot locked as resolved and limited conversation to collaborators Jul 31, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

MyPy linting output not being shown in Problems tab

5 participants