python-zk / kazoo

Kazoo is a high-level Python library that makes it easier to use Apache Zookeeper.
https://kazoo.readthedocs.io
Apache License 2.0
1.3k stars 386 forks source link

chore: add mypy to the build #689

Closed bringhurst closed 1 year ago

bringhurst commented 1 year ago

Why is this needed?

This is intended to be a lightweight version of https://github.com/python-zk/kazoo/pull/684. Rather than add types to everything in one shot, support for typing could be added to the build only. Python code typing could be incrementally added in future smaller/targeted PRs.

The changes in https://github.com/python-zk/kazoo/pull/684 are still useful. This is just an effort to split it up to make it easier to understand.

The method used here follows the same pattern that SQLAlchemy has been using to add types. For an example, see https://github.com/sqlalchemy/sqlalchemy/pull/9039/files and note the removal of the # mypy: ignore-errors comment to incrementally add types.

Proposed Changes

Does this PR introduce any breaking change?

No. This PR will only contain build-time type checking.

codecov-commenter commented 1 year ago

Codecov Report

Base: 94.59% // Head: 94.67% // Increases project coverage by +0.08% :tada:

Coverage data is based on head (e89a57e) compared to base (491cab3). Patch has no changes to coverable lines.

:mega: This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #689 +/- ## ========================================== + Coverage 94.59% 94.67% +0.08% ========================================== Files 57 57 Lines 8339 8339 ========================================== + Hits 7888 7895 +7 + Misses 451 444 -7 ``` | [Impacted Files](https://codecov.io/gh/python-zk/kazoo/pull/689?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=python-zk) | Coverage Δ | | |---|---|---| | [kazoo/handlers/utils.py](https://codecov.io/gh/python-zk/kazoo/pull/689?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=python-zk#diff-a2F6b28vaGFuZGxlcnMvdXRpbHMucHk=) | `94.06% <0.00%> (-0.46%)` | :arrow_down: | | [kazoo/tests/test\_lock.py](https://codecov.io/gh/python-zk/kazoo/pull/689?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=python-zk#diff-a2F6b28vdGVzdHMvdGVzdF9sb2NrLnB5) | `98.78% <0.00%> (-0.18%)` | :arrow_down: | | [kazoo/protocol/connection.py](https://codecov.io/gh/python-zk/kazoo/pull/689?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=python-zk#diff-a2F6b28vcHJvdG9jb2wvY29ubmVjdGlvbi5weQ==) | `97.10% <0.00%> (+0.61%)` | :arrow_up: | | [kazoo/client.py](https://codecov.io/gh/python-zk/kazoo/pull/689?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=python-zk#diff-a2F6b28vY2xpZW50LnB5) | `98.40% <0.00%> (+0.63%)` | :arrow_up: | | [kazoo/tests/test\_eventlet\_handler.py](https://codecov.io/gh/python-zk/kazoo/pull/689?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=python-zk#diff-a2F6b28vdGVzdHMvdGVzdF9ldmVudGxldF9oYW5kbGVyLnB5) | `89.61% <0.00%> (+1.09%)` | :arrow_up: | Help us with your feedback. Take ten seconds to tell us [how you rate us](https://about.codecov.io/nps?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=python-zk). Have a feature suggestion? [Share it here.](https://app.codecov.io/gh/feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=python-zk)

:umbrella: View full report at Codecov.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.

bringhurst commented 1 year ago

Hey @a-ungurianu -- please let me know if you have any thoughts on splitting up the type hinting PR like this. :)

a-ungurianu commented 1 year ago

Hey @a-ungurianu -- please let me know if you have any thoughts on splitting up the type hinting PR like this. :)

Haven't looked at the content of the PR, but conceptually I have no issues with it. Will review later today.

a-ungurianu commented 1 year ago

Thanks a lot for the PR!

When it comes to introducing typings to a new repo, I've seen 2 orthogonal approaches, which is either rule by rule, or module by module.

In the past I've found success with the later approach, as it defines the target a bit more thoroughly and it also creates what I like to call a "ball of coherency", where we can (mostly) trust the interactions between the typed modules.

In theory I wouldn't be against this approach either, but having relatively lax rules might result in us publishing types that aren't accurate.

bringhurst commented 1 year ago

That sounds good to me.

I was looking around at how some other projects are doing it -- it looks like SQLAlchemy is adding a type ignore to the top of each file, then slowly removing it as they go.

I just updated this PR to use that method.

a-ungurianu commented 1 year ago

If we want to go with the file-by-file approach, then we need to make the mypy requirements stricter.

I've pulled the broken bit out of #684 so it should be ready to review. This utilizes the configuration that mypy offers to configure which files are checked or not. I think I like the approach this PR offers for ignoring the files, as it is very visible when editing the file, whereas the mypy configuration is a bit farther away.

Would you be open to get the rule list from #684? After that we can merge this and then rebase #684 and also unlock the ability to parallelize the type checking between multiple contributors.

bringhurst commented 1 year ago

I've pulled the broken bit out of https://github.com/python-zk/kazoo/pull/684 so it should be ready to review. This utilizes the configuration that mypy offers to configure which files are checked or not. I think I like the approach this PR offers for ignoring the files, as it is very visible when editing the file, whereas the mypy configuration is a bit farther away.

I just switched it to use the centralized mypy config method. However, I ended up inverting the list of files to ignore so the config file will shrink in size over time (rather than grow to include all files in the repo).

If we want to go with the file-by-file approach, then we need to make the mypy requirements stricter.

I just went through the mypy config line-by-line (based on the mypy docs) and made it as strict as possible (with the exception of disallow_any_expr, which looks like it's going to be unreasonably noisy until all external packages have types). If it turns out to be too strict, we can always relax it later. :)

Regarding ignores for external packages, for example:

module = 'gevent.*'
module = 'eventlet.*'
module = 'puresasl.*'

We should probably depend on using # type: ignore comments on the import of modules with missing types -- along with the warn_unused_ignores = true flag set. That way, when the external package is upgraded to a version which includes typing, the build will fail with a decent error message until the # type: ignore comment is removed. If these external packages are ignored in the centralized config, types will silently be ignored when the external package is upgraded to a version which includes types (until someone notices somehow and removes it from the mypy.ini).

a-ungurianu commented 1 year ago

Do you have a strong opinion pro having the separate mypy.ini file? I tend to not be a huge fan of the proliferation of dot-files, so given that we can now fold it into pyproject.yaml (see #684), I'd tend towards that

jeffwidman commented 1 year ago

Thanks guys for working on this, I am not closely following the implementation details, but at a high-level I'm 👍 👍 !

bringhurst commented 1 year ago

Do you have a strong opinion pro having the separate mypy.ini file? I tend to not be a huge fan of the proliferation of dot-files, so given that we can now fold it into pyproject.yaml (see https://github.com/python-zk/kazoo/pull/684), I'd tend towards that

No strong preference at all -- I just updated the PR to have the config in the pyproject.toml instead (and also checked to make sure it still works). :)

jeffwidman commented 1 year ago

Thanks again @bringhurst 🎉