Skip to content

feat: Add type hinting#791

Open
ThosRTanner wants to merge 1 commit into
python-zk:masterfrom
ThosRTanner:add-typing-hints
Open

feat: Add type hinting#791
ThosRTanner wants to merge 1 commit into
python-zk:masterfrom
ThosRTanner:add-typing-hints

Conversation

@ThosRTanner
Copy link
Copy Markdown
Contributor

@ThosRTanner ThosRTanner commented May 16, 2026

This adds nearly full type hinting, and enables all but two of the checks that were enabled in the mypy section in pyproject.toml. I hope to address the remaining checks, but it was proving tricky to address some of the typing.Any uses without code changes.

Also if you enable deprecation checks with --enable-error-code=deprecated, you'll get warnings if you use deprecated options (at least as far as I was able to spot them!)

This addresses #647 but there's not entirely strict checking as yet.

Although I've tried very hard not to change any code for this, some return None statements have been added, which has caused the code coverage to drop as apparently those return paths were not covered by the testing.

Also I've been less polite with the testing, and have changed the code in a number of places, including dropping test classes that did nothing, and adding asserts here and there to keep mypy happy as it sometimes doesn't spot a value could have been changed.

I've added FIXMEs to indicate some of the more egregious type warning suppression that I did to avoid doing code changes, and intend to address those in another PR.

I've also added tests for the serialization.py module as some of it is a little unexpected and at one point I'd managed to break it but the resultant errors were very very unclear.

A note: I've also had to suppress some errors in flake8 and use more general #type: ignore suppressions than I'd like because hound CI does something very very odd and tries to parse the text in the [] of the #type: ignore[] comments as python, and generates some very peculiar messages.

Why is this needed?

Type hinting makes it much easier for users to ensure they have the right parameters for functions.

Proposed Changes

  • Type hints for all the public APIs
  • Type hints for all the private APIs
  • Updated tests to use appropriate type hints or in one or two cases fix tests that were doing something peculiar.

Does this PR introduce any breaking change?

Well, it might break people who're already using mypy. And it's entirely possible I've got the wrong type hints (though hopefully actually using the type hinting in the tests has pulled all those out - it certainly picked up a couple of places where I'd got it wrong).

Codecov notes:

  1. eventlet.py - this says it's missing coverage for the eventlet event_object, lock_object and rlock_object.
    • There are only 2 places in the tests that use the SequentialEventletHandler
      • test_eventlet_handler which doesn't have enough in it to trigger locking
      • test_cache.py which will use SequentialEventletHander if it can't find SequentialGeventHandler - which as far as I can see it always will.
    • so I'm not sure how it can have achieved any coverage before.
  2. connection.py - Apparently it doesn't drop out of the main loop in next_server. However, the only way I can see it doing that is if the hosts list is empty, which I don't think is a reasonable situation. Additionally, codecov on earlier code was showing the clearly unreachable code before the end of the loop as being covered.
  3. utils.py - Caused by an explicit return None at the end of a function instead of the implicit one. Cannot tell if this path was ever covered.
  4. lock.py - Caused by an explicit return None at the end of a function instead of the implicit one. Cannot tell if this path was ever covered.

@ThosRTanner ThosRTanner marked this pull request as draft May 17, 2026 12:46
@codecov
Copy link
Copy Markdown

codecov Bot commented May 17, 2026

Codecov Report

❌ Patch coverage is 99.11308% with 8 lines in your changes missing coverage. Please review.
✅ Project coverage is 95.53%. Comparing base (86e69f2) to head (3a14bc1).
⚠️ Report is 10 commits behind head on master.

Files with missing lines Patch % Lines
kazoo/handlers/eventlet.py 91.89% 3 Missing ⚠️
kazoo/protocol/connection.py 95.89% 3 Missing ⚠️
kazoo/handlers/utils.py 97.87% 1 Missing ⚠️
kazoo/recipe/lock.py 97.91% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #791      +/-   ##
==========================================
- Coverage   96.67%   95.53%   -1.15%     
==========================================
  Files          27       27              
  Lines        3553     3871     +318     
==========================================
+ Hits         3435     3698     +263     
- Misses        118      173      +55     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@ThosRTanner ThosRTanner marked this pull request as ready for review May 18, 2026 21:29
This adds nearly full type hinting, and enables all but two of the mypy
checks that were enabled in the mypy section in pyproject.toml. I hope
to address the remaining checks, but it was proving tricky to address
some of the 'typing.Any' uses.

Also if you enable deprecation checks with

--enable-error-code=deprecated

You'll get warnings if you use deprecated options (at least as far as I
was able to spot them!)

This addresses python-zk#647 but there's not entirely strict checking as yet.

Although I've tried very hard not to change any code for this, some
`return None` statements have been added, which has caused the code
coverage to drop as apparently those return paths were not covered
by the testing.

Also I've been less polite with the testing, and have changed the code
in a number of places, including dropping test classes that did nothing,
and adding asserts here and there to keep mypy happy as it sometimes
doesn't spot a value could have been changed.

I've added FIXMEs to address some of the more egregious type warning
suppression that I did to avoid doing code changes, and intend to
address those in another PR.

A note: I've also had to suppress some errors in flake8 and use more
general 'type: ignore' suppressions than I'd like because hound CI
does something very very odd and tries to parse the text in the []
of the type: ignore[] comments as python, and generates some very
peculiar messages.
@StephenSorriaux
Copy link
Copy Markdown
Member

Hello,

Thanks a lot for the PR and the issues you tagged along the way, it's really appreciated to have some fresh eyes on the project.

This PR is quite big so I tagged the @python-zk/maintainers team to have feedback. As for me, I am not really a use-typing-in-Python guy. When I need typing, I use another language, but I know those changes are very valuable for the community. I will review it in the next days as I have some time off.

Special pings to @a-ungurianu and @bringhurst: since you worked on typing features in the past, it would be really appreciated if you can chime in and add a review.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants