ci: add mypy for static type checking #1101
Conversation
leseb
requested review from
ashwinb,
yanxi0830,
hardikjshah,
dltn,
raghotham,
dineshyv,
vladimirivic,
sixianyi0721,
ehhuang and
terrytangyuan
as code owners
facebook-github-bot
added
the
CLA Signed
leseb
force-pushed
the
mypyy
branch
2 times, most recently
from
461e5cc to
d45030d
Compare
|
Hm, this one is a biggy. I am OK with CI running this but local pre-commits should be much faster or at least there should be an option for a developer to avoid adding this all the time. Do you know how much additional time it ends up taking? |
|
We do need to add some light type checking for sure though |
On my local machine, it takes a few extra seconds. The initial run is longer due to cache initialization, but it’s not too bad. Plus, since we’re skipping most of the codebase, the impact is minimal. EDIT: also, while running pre-commit before pushing is recommended, it's still optional, if triggered from a git hook, one can still use |
requirements.txt
Outdated
| @@ -12,7 +12,7 @@ distro==1.9.0 | |||
| exceptiongroup==1.2.2 ; python_full_version < '3.11' | |||
| filelock==3.17.0 | |||
| fire==0.7.0 | |||
| fsspec==2024.12.0 | |||
There was a problem hiding this comment.
I guess it's "fine" though probably has nothing to do with mypy enablement. :)
.pre-commit-config.yaml
Outdated
| hooks: | ||
| - id: mypy | ||
| name: mypy | ||
| entry: bash -c 'uv sync --extra dev && uv run mypy' |
There was a problem hiding this comment.
can this just be uv run mypy? see the pre-commit entry added by @bbrowning as an example.
| # type: Literal["custom"] = "custom" | ||
| # validator_class: str | ||
| class CustomType(BaseModel): | ||
| pylint: disable=syntax-error |
There was a problem hiding this comment.
Why is this needed if all of this is inside a string? Why was it even needed to be changed from a comment to a quoted string?
There was a problem hiding this comment.
mypy flagged type: Literal["custom"] = "custom" as an issue, even though it was inside a comment. Wrapping the block in a docstring made it invisible to MyPy, preventing the error.
| @@ -21,6 +19,13 @@ class WebMethod: | |||
| method: Optional[str] = None | |||
|
|
|||
|
|
|||
| class HasWebMethod(Protocol): | |||
| # # honor excludes by not following there through imports | ||
| follow_imports = "silent" | ||
| exclude = [ | ||
| # As we fix more and more of these, we should remove them from the list |
- Enable mypy to run in the CI on a subset of the repository - Fix a few mypy errors - Run mypy from pre-commit Signed-off-by: Sébastien Han <[email protected]>
What does this PR do?
Signed-off-by: Sébastien Han [email protected]
Test Plan
[Describe the tests you ran to verify your changes with result summaries. Provide clear instructions so the plan can be easily re-executed.]