Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added type annotation to public classes. #1074

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

jackgene
Copy link
Contributor

@jackgene jackgene commented Nov 15, 2024

Changes

Adds type annotation to public classes, and py.typed so that client code can type check against this library.

Fixes #980 (kind of)

Note that the issue in question seems to be more ambitious in that it aims to provide type information to the entire library:

  1. Allowing bugs to be discovered
  2. Allowing developers consuming this library to have better type information to work with.

This PR only aims to solve the latter.

Per providing type annotations, there are several ways to distribute type information:

  • inline type annotations (preferred)
  • type stub files included in the package
  • a separate companion type stub package
  • type stubs in the typeshed repository

This is the first option, and makes changes to .py files mostly just adding type annotations. If you prefer to go with the second option (which doesn't touch any .py file), please have a look at #1075 instead.

Details about this PR:

  • Using Python 3.10+ typing, which is available in 3.9 via from __future__ import annotations
    • x | None instead of Optional[x]
    • x | y instead of Union[x, y]
    • list[x]/set[x]/dict[x, y] instead of typing.List[x]/typing.Set[x]/typing.Dict[x, y]
  • For the most part, all I'm doing is explicitly annotate methods as documented in docstring
  • There are special cases though where the docstring had to be updated, e.g.,:
    • docstring says list, but varargs/default value is tuple
    • docstring doesn't say optional, but default value is None

Note that this is introducing a breaking change that may affect a small minority of users. The key/value serializer/deserializer functions no longer optional (with the identity function defined as the default parameter). This is so that the generic parameters KT/VT can be inferred.

This means that:

  • The (de)serialization behavior is identical at runtime
  • As long as clients do not explicitly set None as their key/value serializer/deserializer function, there should be no change.

However, if a client is explicitly passing in None, e.g., with AIOKafkaConsumer:

# OK, most people probably do this anyway, this is from the README
consumer = AIOKafkaConsumer(
    'my_topic', 'my_other_topic',
    bootstrap_servers='localhost:9092',
    group_id='my-group'
)

# Not OK, key_deserializer/value_deserializer cannot be None
consumer = AIOKafkaConsumer(
    'my_topic', 'my_other_topic',
    bootstrap_servers='localhost:9092',
    group_id='my-group',
    key_deserializer=None,
    value_deserializer=None
)

Or with AIOKafkaProducer:

# OK, most people probably do this anyway, this is from the README
AIOKafkaProducer(
    bootstrap_servers='localhost:9092'
)

# Not OK, key_deserializer/value_deserializer cannot be None
AIOKafkaProducer(
    bootstrap_servers='localhost:9092'
    key_serializer=None,
    value_serializer=None
)

The type checker will reject it.
image

You can ignore the type checker though, and the application should work the same as before, as this PR does not change any runtime logic.

Checklist

  • I think the code is well written
  • Unit tests for the changes exist
  • Documentation reflects the changes
  • Add a new news fragment into the CHANGES folder
    • name it <issue_id>.<type> (e.g. 588.bugfix)
    • if you don't have an issue_id change it to the pr id after creating the PR
    • ensure type is one of the following:
      • .feature: Signifying a new feature.
      • .bugfix: Signifying a bug fix.
      • .doc: Signifying a documentation improvement.
      • .removal: Signifying a deprecation or removal of public API.
      • .misc: A ticket has been closed, but it is not of interest to users.
    • Make sure to use full sentences with correct case and punctuation, for example: Fix issue with non-ascii contents in doctest text files.


from aiokafka import errors as Errors
from aiokafka.client import CoordinationType

Check failure

Code scanning / CodeQL

Module-level cyclic import Error

'CoordinationType' may not be defined if module
aiokafka.client
is imported before module
aiokafka.cluster
, as the
definition
of CoordinationType occurs after the cyclic
import
of aiokafka.cluster.
@jackgene jackgene mentioned this pull request Nov 15, 2024
4 tasks
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.

Proposal to Add Type Hints
1 participant