Page MenuHomeSoftware Heritage

Add a mypy precommit hook
ClosedPublic

Authored by douardda on Nov 19 2019, 4:53 PM.

Details

Diff Detail

Repository
rDCORE Foundations and core functionalities
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

douardda created this revision.Nov 19 2019, 4:53 PM
olasd added a subscriber: olasd.Nov 19 2019, 5:32 PM

Sadly this needs all dependencies to be installed in the virtualenv pre-commit uses to run mypy.

Trying out

- repo: local
  hooks:
  - id: mypy
    name: mypy
    entry: mypy
    language: system
    types: [python]

Seems to run mypy as a pre-commit hook on modified python files.

Setting language: system runs mypy directly without isolating it in a venv (so, runs it from the dev venv instead, which likely has the dependencies installed).

You can set entry: mypy swh and pass_filenames: false to run mypy on the whole swh module every time a python file changes.

In fact using:

- repo: https://github.com/pre-commit/mirrors-mypy
  rev: 'master'
  hooks:
  - id: mypy
    args: []

seems to work just fine. The "problem" is it needs several more entries in the mypy.ini file because there are several python packages missing in the venv mypy run from (namely aiohttp, hypothesis, swh.docs and setuptools).

With this config above (+ fixes in mypy.ini) and

swh-core$ git diff
diff --git a/swh/core/api/__init__.py b/swh/core/api/__init__.py
index 63ea3e0..a07e445 100644
--- a/swh/core/api/__init__.py
+++ b/swh/core/api/__init__.py
@@ -101,7 +101,7 @@ def remote_api_endpoint(path):
 
 class APIError(Exception):
     """API Error"""
-    def __str__(self):
+    def __str__(self) -> int:
         return ('An unexpected error occurred in the backend: {}'
                 .format(self.args))

results in:

git commit swh/core/api/__init__.py 
Trim Trailing Whitespace.................................................Passed
Flake8...................................................................Passed
Check JSON...........................................(no files to check)Skipped
Check Yaml...........................................(no files to check)Skipped
codespell................................................................Passed
mypy.....................................................................Failed
hookid: mypy

swh/core/api/__init__.py:104: error: Return type "int" of "__str__" incompatible with return type "str" in supertype "BaseException"
swh/core/api/__init__.py:104: error: Return type "int" of "__str__" incompatible with return type "str" in supertype "object"
swh/core/api/__init__.py:105: error: Incompatible return value type (got "str", expected "int")
Found 3 errors in 1 file (checked 1 source file)

which looks quite ok to me.

douardda updated this revision to Diff 7951.Nov 20 2019, 10:12 AM

add 'args: []' in mypy's pre-commit config and fix mypy.ini for missing stubs

zack requested changes to this revision.Nov 20 2019, 10:54 AM
zack added a subscriber: zack.

seems to work just fine.
[...]

Found 3 errors in 1 file (checked 1 source file)

I don't think this is good enough, in the sense that in this test you've introduced a type error in the file that is also the only one modified here, whereas modifying one file might introduce type errors in *other* files.
What we want here is to always run mypy swh as a pre-commit test, no matter which files have been modified.

@olasd suggested above to use entry: mypy swh and pass_filenames: false as a way to achieve that. I don't know if that's the way to make it work, but I maintain that the behavior we want is what I've described above.

This revision now requires changes to proceed.Nov 20 2019, 10:54 AM
In D2311#54287, @zack wrote:

seems to work just fine.
[...]

Found 3 errors in 1 file (checked 1 source file)

I don't think this is good enough, in the sense that in this test you've introduced a type error in the file that is also the only one modified here, whereas modifying one file might introduce type errors in *other* files.
What we want here is to always run mypy swh as a pre-commit test, no matter which files have been modified.

Ah ok, sure.

@olasd suggested above to use entry: mypy swh and pass_filenames: false as a way to achieve that. I don't know if that's the way to make it work, but I maintain that the behavior we want is what I've described above.

indeed.

douardda updated this revision to Diff 7957.Nov 20 2019, 12:24 PM

enforce running mypy on the whole swh package each time

zack accepted this revision.Nov 20 2019, 12:41 PM
This revision is now accepted and ready to land.Nov 20 2019, 12:41 PM
douardda updated this revision to Diff 7962.Nov 20 2019, 2:42 PM

use '.[testing]' as additional dep

also keep entries in mypy.ini sorted.

This revision was landed with ongoing or failed builds.Nov 21 2019, 11:07 AM
This revision was automatically updated to reflect the committed changes.