Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Should we enforce type checking in CI? #321

Open
MridulS opened this issue Apr 5, 2024 · 3 comments
Open

Should we enforce type checking in CI? #321

MridulS opened this issue Apr 5, 2024 · 3 comments

Comments

@MridulS
Copy link
Member

MridulS commented Apr 5, 2024

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
Copy link
Member

nvaytet commented Apr 5, 2024

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
Copy link
Member Author

MridulS commented Apr 17, 2024

Just ran on main branch. We are currently on

...
...
Found 2097 errors in 106 files (checked 115 source files)

@jl-wynen
Copy link
Member

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 scipp/scipp#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)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants