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

docs: kick start the process of adding type definitions #684

Open a-ungurianu opened 1 year ago

a-ungurianu commented 1 year ago

Beginnings of #647

Why is this needed?

Python type annotations are a great way of documenting the APIs, as well as improving the developer experience when using this library.

I find the actual type checking to be an added benefit than a necessity.

Proposed Changes

Does this PR introduce any breaking change?

ceache commented 1 year ago

Hi @a-ungurianu,

Awesome that you started that work!!

Full disclosure, I haven't reviewed all the changes but one thing I wanted to mention is trying to minimize imports at runtime. 

In most of my projects, i am able to only import TYPE_CHECKING and occasionally cast like so:


from typing import TYPE_CHECKING

# ... other imports

if TYPE_CHECKING:  # pragma: nocover
    from typing import...
    from typing_extensions import...
    T = TypeVar(...)

I have seen numerous other projects doing that as well.

What do you think?

a-ungurianu commented 1 year ago

I think I agree. I've just been adding any new modules inside the TYPE_CHECKING check, but adding all imports used for typing and repeating module imports makes sense.

codecov-commenter commented 1 year ago

Codecov Report

Attention: Patch coverage is 82.60870% with 28 lines in your changes are missing coverage. Please review.

Project coverage is 96.19%. Comparing base (2fb93a8) to head (07bbc80). Report is 3 commits behind head on master.

Files Patch % Lines
kazoo/client.py 82.50% 7 Missing :warning:
kazoo/recipe/barrier.py 81.81% 4 Missing :warning:
kazoo/recipe/election.py 63.63% 4 Missing :warning:
kazoo/retry.py 50.00% 4 Missing :warning:
kazoo/recipe/counter.py 86.36% 3 Missing :warning:
kazoo/recipe/party.py 90.62% 3 Missing :warning:
kazoo/recipe/lease.py 89.47% 2 Missing :warning:
kazoo/recipe/lock.py 85.71% 1 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #684 +/- ## ========================================== - Coverage 96.81% 96.19% -0.62% ========================================== Files 27 27 Lines 3549 3628 +79 ========================================== + Hits 3436 3490 +54 - Misses 113 138 +25 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

a-ungurianu commented 1 year ago

This is ready for a proper review now.

The history log in the PR is a bit of a mess for this due to me fighting with Hound and with making sure the stuff used from typing is available from 3.7 upwards

takeda commented 7 months ago

@a-ungurianu any chance this would get merged?

a-ungurianu commented 4 months ago

I'm considering declaring bankruptcy on this PR and re-openning so that it looks a bit more approachable for review.

Also, if there's anything in the structure that makes it hard to review, lemme know and I can amend

StephenSorriaux commented 4 months ago

Would it be possible to exclude the if TYPE_CHECKING: lines from the test coverage? So that codecov can give us a "true" coverage. (see https://coverage.readthedocs.io/en/latest/excluding.html#advanced-exclusion) Probably the ... and/or @overload too.