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

Proposal to Add Type Hints #980

Open
alm0ra opened this issue Feb 13, 2024 · 14 comments · Fixed by #999 · May be fixed by #1075 or #1074
Open

Proposal to Add Type Hints #980

alm0ra opened this issue Feb 13, 2024 · 14 comments · Fixed by #999 · May be fixed by #1075 or #1074

Comments

@alm0ra
Copy link
Contributor

alm0ra commented Feb 13, 2024

Describe the solution you'd like
I'd like to propose adding type hints to the Aiokafka project. I've used mypy to analyze the codebase, and I believe that adding type hints could significantly improve the code's readability, maintainability, and robustness.

result for mypy

Found 2475 errors in 109 files (checked 133 source files)

By adding type hints, developers can better understand function signatures and catch potential bugs early in the development process. Additionally, it can make it easier for newcomers to contribute to the project and for existing contributors to navigate the codebase.

I'm willing to contribute to this effort if the maintainers are interested in pursuing it. Adding type hints could be a valuable enhancement to the Aiokafka project.

I can start with aiokafka/consumer/consumer.py and fix all issue in this file first and continue with other files in another PR's or you can assign others to contribute

@alm0ra
Copy link
Contributor Author

alm0ra commented Feb 13, 2024

@ods

@alm0ra alm0ra mentioned this issue Feb 13, 2024
4 tasks
@ods
Copy link
Collaborator

ods commented Feb 14, 2024

Yes, we're interested in adding type hints. I'd like to highlight some issues here.

  • Incorrect types are often worse that missing types
  • Gradual approach doesn't work good here: you never sure types are correct and consistent, until everything is annotated
  • Some features like custom serialization functions don't allow us to provide good annotations. So, we may want to consider deprecating such features first.

@alm0ra
Copy link
Contributor Author

alm0ra commented Feb 14, 2024

Thank you for bringing up those concerns. I understand the importance of ensuring that type hints are accurate and consistent. Regarding the Gradual approach doesn't work good here point, I believe we can still make progress by prioritizing certain areas for annotation while acknowledging that some parts may remain untyped initially.

By leveraging tools like MyPy, we can focus on annotating sections of the codebase where types are clearer, thereby incrementally improving the overall typing coverage. We can then gradually address the more complex or ambiguous sections, such as the custom serialization functions, as we gain more clarity on their typings.

@alm0ra
Copy link
Contributor Author

alm0ra commented Feb 14, 2024

#981 should I close this PR?

@ods
Copy link
Collaborator

ods commented Feb 16, 2024

#981 should I close this PR?

I hope to find time on weekend to look through it.

@alm0ra alm0ra mentioned this issue Feb 17, 2024
4 tasks
@antonagestam
Copy link

Beartype could be of interest here. I've successfully used it in unit tests to increase confidence that type hints are accurate. It can be injected by using an early stage pytest hook.

https://github.com/beartype/beartype

@ods
Copy link
Collaborator

ods commented Mar 11, 2024

@antonagestam, thanks for the suggestion! It looks interesting, definitely worth to try. Do you use it via pytest-beartype or in some other way?

@antonagestam
Copy link

@ods Currently I use the decorator, mostly because the code-base is too large for it to be feasible to introduce it everywhere at once, and we don't want to enable it for all test suites. After some initial acclimatization I hope we'll be able to use one of the import hooks.

@laipz8200
Copy link

Here is the signature of AIOKafkaProducer:

def __init__(self, *, loop=None, bootstrap_servers='localhost', ...)

The bootstrap_servers parameter has a default value of localhost, so the type inference system infers it as a string (and throws an error when a list of strings is passed in).

This can be confusing for many users who use the type system. Can we prioritize adding type annotations for these classes?

@odysa
Copy link

odysa commented Mar 28, 2024

As a user of aiokafka, I like typing hints. I want to contribute to the typing feature. Where should I start from?

@ods
Copy link
Collaborator

ods commented Mar 28, 2024

Hi @odysa, thank you for your willingness to contribute! Probably we need more communication with interested people to discuss problems. Let me highlight some difficult places:

  • It would be nice to have some way of verifying that typing is correct. mypy doesn't work well with partially typed code, as it assume Any everywhere instead of type inference. Probably some way of runtime verification is needed.
  • It's better to start from low-level modules, that don't rely on other untyped parts of aiokafka. One of such packages is ptotocol package. Right now it has with methods in subclasses not following initial signature, which doesn't work when type checked.
  • One of features, an ability to provide custom serializer/deserializer requires making high-level classes generic, which is highly unwanted. We have to decide, what to do with this.

@dimastbk
Copy link
Contributor

@ods please reopen this issue :)

@ods ods reopened this Apr 21, 2024
@ssbarnea
Copy link

Following pep-561 should be mentioned here too.

@jackgene
Copy link
Contributor

jackgene commented Oct 10, 2024

...

  • Some features like custom serialization functions don't allow us to provide good annotations. So, we may want to consider deprecating such features first.

I believe this isn't too difficult, as long as you make AIOKafkaConsumer and AIOKafkaProducer generic, IOW:

KT = TypeVar("KT", covariant=True)
VT = TypeVar("VT", covariant=True)


class AIOKafkaConsumer(Generic[KT, VT]):
    ...
    def __init__(
        self,
        ...
        key_deserializer: Callable[[bytes], KT]=lambda x: x,
        value_deserializer: Callable[[bytes], VT]=lambda x: x,
        ...
def _identity(data: bytes) -> bytes:
    return data


KT = TypeVar("KT", contravariant=True)
VT = TypeVar("VT", contravariant=True)


class AIOKafkaProducer(Generic[KT, VT]):
    ...
    def __init__(
        self,
        ...
        key_serializer: Callable[[KT], bytes]=_identity,
        value_serializer: Callable[[VT], bytes]=_identity,
        ...

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