Configure mypy #1243
Configure mypy #1243
Conversation
| exclude = [ | ||
| "^dandiapi/api/admin.py", | ||
| "^dandiapi/api/mail.py", | ||
| "^dandiapi/api/tests/", | ||
| "^dandiapi/api/management/", | ||
| "^dandiapi/api/migrations/", | ||
| "^dandiapi/api/models/", | ||
| "^dandiapi/api/tasks/", | ||
| "^dandiapi/api/views/", | ||
| ] |
There was a problem hiding this comment.
For now, I've excluded all files and directories that have type errors. I'll make follow up PRs where I fix the type errors in each of these and remove the corresponding entry from exclude.
| [tool.mypy] | ||
| ignore_missing_imports = true | ||
| show_error_codes = true | ||
| disable_error_code = ["attr-defined", "var-annotated"] |
There was a problem hiding this comment.
These two error cases produced a lot of type errors, many of which were incorrect because of django typing stubs not working. I've disabled them for now, and if django typing stubs ever get integrated with django-configurations properly, we can re-enable them.
| # Re-enable these when https://github.com/typeddjango/django-stubs/issues/417 is fixed. | ||
| # plugins = ["mypy_django_plugin.main", "mypy_drf_plugin.main"] | ||
|
|
||
| # [tool.django-stubs] | ||
| # django_settings_module = "dandiapi.settings" |
There was a problem hiding this comment.
Since we are using django-configurations (well, technically composed-configuration does, not dandi), there's some additional considerations for running these django plugins. There's an open issue to support it typeddjango/django-stubs#417, but it hasn't been implemented yet
There was a problem hiding this comment.
DANDI does use django-configurations directly. That's why things like this: https://github.com/dandi/dandi-archive/blob/master/manage.py#L14 are necessary.
I agree that due to the lack of proper support, it's safer to disable these plugins for now.
tox.ini
Outdated
| skip_install = true | ||
| setenv = | ||
| DJANGO_CONFIGURATION = DevelopmentConfiguration | ||
| passenv = |
There was a problem hiding this comment.
If we were using the Mypy plugin, then the settings file would be getting parsed and these should be necessary. However, this shouldn't be necessary just to import all the files. Do you know what is requiring these?
There was a problem hiding this comment.
Yeah that's exactly why - I was using mypy_django_plugin previously, before I realized it didn't work with django-configuration - updated
There was a problem hiding this comment.
So can this be removed now?
There was a problem hiding this comment.
My bad, I thought you were referring to the setenv section here, not passenv. But yes, passenv can also be removed now, I've pushed that change.
mvandenburgh
force-pushed
the
configure-mypy
branch
from
16e815e
to
fe622a8
Compare
mvandenburgh
force-pushed
the
configure-mypy
branch
from
fe622a8
to
f81121b
Compare
tox.ini
Outdated
| deps = | ||
| mypy | ||
| django-stubs | ||
| djangorestframework-stubs | ||
| types-setuptools |
There was a problem hiding this comment.
| types-setuptools |
We shouldn't need this. We're not checking setup.py, we're looking directly inside dandiapi/.
There was a problem hiding this comment.
We're using setuptools_scm for versioning in this file https://github.com/dandi/dandi-archive/blob/master/dandiapi/__init__.py. We could ignore it, but I don't think it hurts that much either to add it as a dependency. 🤷 I added it back as a dev extra dependency, but I'm fine with just excluding this one file too if installing types-setuptools is too overkill - any thoughts @brianhelba @AlmightyYakob ?
There was a problem hiding this comment.
I didn't realize you had that import of pkg_resources in your actual project module. I think it's correct to include types-setuptools for that. Sorry for my confusion.
@AlmightyYakob This is totally out of scope of this PR, but note there's a way to get the package version from Python 3.8+ standard libraries (rather than setuptools): https://github.com/pypa/setuptools_scm/#retrieving-package-version-at-runtime
mvandenburgh
force-pushed
the
configure-mypy
branch
from
f81121b
to
feb21e5
Compare
mvandenburgh
force-pushed
the
configure-mypy
branch
from
feb21e5
to
6f174b0
Compare
mvandenburgh
force-pushed
the
configure-mypy
branch
from
6f174b0
to
58389b4
Compare
There was a problem hiding this comment.
LGTM. 
@mvandenburgh Thanks for your perseverance in working out some of these low-level usage details. I'd like to upstream some of these change to the Girder 4 cookiecutter and I've learned a lot myself from this experience.
|
I'll ping @waxlamp @danlamanna @AlmightyYakob @DeepikaGhodki here just so you all are aware of this change. In particular, note that this will now run in CI alongside the other tox checks, after linting and before the pytest tests. |
|
🚀 PR was released in |
Closes #1232.
This makes the necessary changes to
tox.inito enablemypystatic type-checking locally and in CI. Type-checking can be run viatox -e type.