search

scipp / plopp

Visualization library for scipp
https://scipp.github.io/plopp/
BSD 3-Clause "New" or "Revised" License
7 stars 5 forks source link

Should we enforce type checking in CI? #321

Open MridulS opened 6 months ago

MridulS commented 6 months ago

There are type annotations in the codebase but it is not being enforced. Running python -m mypy . fails with 

tests/conftest.py: error: Duplicate module named "conftest" (also at "./tests/backends/plotly/conftest.py")
tests/conftest.py: note: See https://mypy.readthedocs.io/en/stable/running_mypy.html#mapping-file-paths-to-modules for more info
tests/conftest.py: note: Common resolutions include: a) using `--exclude` to avoid checking one of them, b) adding `__init__.py` somewhere, c) using `--explicit-package-bases` or adjusting MYPYPATH
nvaytet commented 6 months ago

An in-person discussion is probably better here. We talked about this several times, and the conclusion was that for now, we felt getting mypy to pass everywhere could be more of a hindrance, as it often shows quite a few false-positives. That said, it does also find bugs sometimes.

That decision wasn't final, and we are maybe slowly changing our minds on this.

In any case, I think the state of the type-hinting on Plopp is far from perfect/complete, and definitely needs work.

MridulS commented 5 months ago

Just ran on main branch. We are currently on

...
...
Found 2097 errors in 106 files (checked 115 source files)
jl-wynen commented 5 months ago

There are type annotations in the codebase but it is not being enforced. Running python -m mypy . fails with

tests/conftest.py: error: Duplicate module named "conftest" (also at "./tests/backends/plotly/conftest.py")
tests/conftest.py: note: See https://mypy.readthedocs.io/en/stable/running_mypy.html#mapping-file-paths-to-modules for more info
tests/conftest.py: note: Common resolutions include: a) using `--exclude` to avoid checking one of them, b) adding `__init__.py` somewhere, c) using `--explicit-package-bases` or adjusting MYPYPATH

Unfortunately, this seems to be a consequence of https://github.com/scipp/scipp/issues/3344. Mypy in general does not play well with code that is not part of a package. Another problem is that we cannot specify suppressions for tests. E.g., allowing untyped functions so we don't have to put -> None for every test. I tried to fix this back when we remvoed the __init__.py files. But I had no success.

So I'd say we either use manual workarounds (like excluding one conftest.py file) or simply don't run mypy on tests. The latter is in principle fine as tests are about checking runtime behaviour. But this means that we have no type checks that check whether the different components of the package fit together. (Sort of integration type checks)