chore: add mypy to the build

[bringhurst]

Why is this needed?

This is intended to be a lightweight version of #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 #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

• Add mypy configuration to the build to allow for incremental addition of type annotations.

Does this PR introduce any breaking change?

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

[codecov-commenter]

Codecov Report

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

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

📣 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

@@            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	Coverage Δ
kazoo/handlers/utils.py	94.06% <0.00%> (-0.46%)	⬇️
kazoo/tests/test_lock.py	98.78% <0.00%> (-0.18%)	⬇️
kazoo/protocol/connection.py	97.10% <0.00%> (+0.61%)	⬆️
kazoo/client.py	98.40% <0.00%> (+0.63%)	⬆️
kazoo/tests/test_eventlet_handler.py	89.61% <0.00%> (+1.09%)	⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

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

[a-ungurianu]

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]

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]

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]

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]

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.

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]

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

[a-ungurianu]

I like this. Thanks for adding the reference link. Always takes me 5 min or so to find the reference 😛

[jeffwidman]

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

[bringhurst]

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

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). :)

[a-ungurianu]

LGTM. Thanks a lot and sorry for the back and forth :)

[jeffwidman]

This looks very reasonable to me, but I haven't been working professionally in Python the last few years, so haven't kept up with the idiomatic type tooling recommendations... so before we merge, I'd like to get @ofek's input, as I'm pretty sure he's stayed abreast of such things. I pinged him for a quick review, and assuming he's 👍 then we should merge.

[ofek]

Looks good to me!

[jeffwidman]

Thanks again @bringhurst 🎉