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

AVRO-2921: Strict Type Checking #953

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

kojiromike
Copy link
Contributor

@kojiromike kojiromike commented Aug 30, 2020

This is the first pass at adding type hints across all of avro in python.

  • It adds strict = true to mypy configuration, which will help ensure future changes are consistent, type-wise.
  • In some cases I added # type: ignore comments to avoid having to make API breaking changes to fix type errors.
  • I opted to use stub files to avoid modifying existing code where possible. (Embedded type hints seem to integrate with tooling better, so we may want to change this later.)

Jira

  • My PR addresses AVRO-2921
  • …and adds no dependencies

Tests

  • My PR adds type hints to runtime code and tests.

Commits

  • My commits all reference Jira issues in their subject lines.

Documentation

  • My PR does not add new functionality.

@kojiromike kojiromike force-pushed the AVRO-2921/typehint branch 2 times, most recently from e1cd422 to c82fb1f Compare August 30, 2020 14:17
@Fokko
Copy link
Contributor

Fokko commented Nov 1, 2020

I like this, what's the status?

@kojiromike
Copy link
Contributor Author

To be honest, I am a little stuck. It seems to me there are places where the types in avro-python aren't mathematically sound and can't be correctly described using type hints. (In particular, the subclassing of types of Schema don't always graph right.) In the long run, I think a breaking refactor will be required to make those type-able.

But I guess we can ignore those for now. The bigger issue is that mypy doesn't support recursive types, which I think severely limits the usefulness of type hinting in Avro. Avro schema is obviously suited to a recursive description of what it can serialize.

I've played with a bunch of approaches, including an arbitrary recursion-depth limit (which Python has anyway for stack recursion), but it's a bit depressing, so I took a step away from this for a while.

Mypy continues to improve, so I hope that with future improvements this will become increasingly useful.

In the mean time, I will see about fixing it up and merging it for partial advances in type hinting from avro-python.

@kojiromike kojiromike force-pushed the AVRO-2921/typehint branch from c178395 to 5aca8e2 Compare June 1, 2021 03:33
@asosnovsky-sumologic
Copy link

@kojiromike at least having some top level type-checking is till very useful for catching errors.
Also the issue you referenced is now resolved :) python/mypy#731

@kojiromike
Copy link
Contributor Author

@asosnovsky-sumologic Yep, I saw that and will see about getting back into this. Pretty busy in my personal life, so others are welcome to give it a shot if you can get it done faster than I can.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants