Add mypy type checking to CI (#226) - #2398
Conversation
Signed-off-by: AravCS <aravshah927@gmail.com>
Signed-off-by: AravCS <aravshah927@gmail.com> Co-authored-by: tristonfgibson <246780826+tristonfgibson@users.noreply.github.com> Co-authored-by: yaeger211202 <163278221+yaeger211202@users.noreply.github.com>
|
Hi @yaeger211202 and @tristonfgibson, I think it may be a good idea to use I think the best way to discuss this in more detail is to create an issue in https://github.com/aboutcode-org/skeleton/. If it proves useful, we can integrate it into all of our projects. |
|
Hi @ziadhany, thanks for the reply! As you suggested, I've opened an issue in skeleton: aboutcode-org/skeleton#106 |
| files = | ||
| # fetch and parse vulnerability data from upstream sources | ||
| vulnerabilities/importers/debian_oval.py, | ||
| vulnerabilities/importers/epss.py, | ||
| vulnerabilities/importers/project_kb_msr2019.py, | ||
| vulnerabilities/importers/redhat.py, | ||
| vulnerabilities/importers/suse_scores.py, | ||
| vulnerabilities/importers/ubuntu_usn.py, | ||
| vulnerabilities/importers/xen.py, |
There was a problem hiding this comment.
I think we should use an exclude list instead. Adding every file individually isn't a good option, especially because whenever we create a new file, it will require manually updating the mypy configuration.
There was a problem hiding this comment.
done, switched to using an exclude list (mypy uses regex syntax for the exclude list), so now whenever there is a new file created, it is type checked automatically.
| mypy: | ||
| @echo "-> Run mypy type checking" | ||
| @${ACTIVATE} mypy | ||
|
|
There was a problem hiding this comment.
I think mypy command should also be part of check command
There was a problem hiding this comment.
done, added mypy command to the check command as well, as suggested. Also, the make check is commented out currently in main.yml, so I left the standalone mypy command as well.
| warn_unused_ignores = True | ||
|
|
||
| strict_equality = True | ||
| warn_return_any = True |
There was a problem hiding this comment.
There are a lot of options available. The main question is whether any of them work well with django without requiring a plugin.
There was a problem hiding this comment.
Hi, the config options alone can’t completely solve this, they control how strict mypy is, but can’t give it knowledge of Django’s ORM. django-stubs works for this and gives more useful type checking, but it requires a mypy plugin.
At the moment, without django-stubs installed, mypy treats Django as untyped, so it primarily checks regular Python code it can understand, rather than type checking Django-specific APIs. With the ignore_missing_imports = True flag that we included, mypy suppresses errors about missing type information for imports, such as Django.
Signed-off-by: AravCS <aravshah927@gmail.com>
…into add-mypy-ci
| # test suite not yet typed | ||
| | ^vulnerabilities/tests/pipelines/test_base_pipeline\.py$ | ||
| | ^vulnerabilities/tests/pipelines/test_compute_advisory_todo\.py$ | ||
| | ^vulnerabilities/tests/pipelines/test_npm_importer_pipeline\.py$ | ||
| | ^vulnerabilities/tests/pipelines/test_pipeline_id\.py$ | ||
| | ^vulnerabilities/tests/pipelines/test_pypa_importer_pipeline\.py$ | ||
| | ^vulnerabilities/tests/pipelines/test_pysec_importer_pipeline\.py$ | ||
| | ^vulnerabilities/tests/pipelines/test_remove_duplicate_advisories\.py$ | ||
| | ^vulnerabilities/tests/pipelines/v2_importers/test_apache_httpd_importer_pipeline_v2\.py$ | ||
| | ^vulnerabilities/tests/pipelines/v2_importers/test_apache_kafka_importer\.py$ | ||
| | ^vulnerabilities/tests/pipelines/v2_importers/test_apache_tomcat_importer_pipeline\.py$ | ||
| | ^vulnerabilities/tests/pipelines/v2_importers/test_collect_fix_commit\.py$ | ||
| | ^vulnerabilities/tests/pipelines/v2_importers/test_debian_importer\.py$ | ||
| | ^vulnerabilities/tests/pipelines/v2_importers/test_elixir_security_importer_v2\.py$ | ||
| | ^vulnerabilities/tests/pipelines/v2_importers/test_github_osv_importer_v2\.py$ | ||
| | ^vulnerabilities/tests/pipelines/v2_importers/test_gitlab_importer_v2\.py$ | ||
| | ^vulnerabilities/tests/pipelines/v2_importers/test_istio_importer_v2\.py$ | ||
| | ^vulnerabilities/tests/pipelines/v2_importers/test_mattermost_importer_v2\.py$ |
There was a problem hiding this comment.
I think it would be simpler to exclude the tests using one or two regex patterns instead of listing them all out individually
There was a problem hiding this comment.
Hi @ziadhany, I’m happy to make those changes, I just wanted to make sure of the following before. If I make the regex more concise like you mentioned, I would have to exclude the test directories more broadly which would cause us to lose coverage on the test files that are clean.
Right now, roughly 96 files are clean, and 54 have errors. There doesn't seem to be a simple way to make it just one or two regex patterns while keeping the coverage, because the preexisting typing errors are spread out in the test directory.
I’m happy to just make it concise though with a broader one line regex, let me know what you prefer.
3 participants
Hello me and my team, @yaeger211202 and @tristonfgibson worked on this as part of CodeDay Labs. Here is a summary of our changes:
Added mypy type checking to CI. Consists of already-clean, high-value modules as the initial whitelist target.
Rather than running mypy across the whole codebase (pre-existing errors), it uses a whitelist in mypy.ini, only the listed files are checked. This follows mypy’s existing-codebase docs by starting on a clean subset and expanding over time. Config uses mypy’s recommended foundation settings as well.
Changes pass both locally and on Github Actions (run on a fork against current main).
Changes: