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

Add mypy strict typing #140

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Add mypy strict typing #140

wants to merge 3 commits into from

Conversation

jhnstrk
Copy link
Contributor

@jhnstrk jhnstrk commented Apr 10, 2024

This PR adds typing sufficient to pass mypy --strict checks.

The changes attempt to keep the public-facing API as close to unchanged as possible.

Internally there are some points that required design choices that are debatable:

  • Use of assert in the code for type narrowing: is this "forbidden" here, or not? A few instances were added here. It may have a small performance impact, and could cause some existing code to fail.
  • Some use of typing.cast. This should not have an impact, but could also be replaced with # type: ignore comments.
  • Strict typing in the tests - currently the source in tests pass mypy, but I'm not sure test code needs to pass type checks.

I also added mypy to the Github actions.

With the changes, the unit tests pass, and performance (parsing throughput) seems unchanged.

@Kludex
Copy link
Owner

Kludex commented Apr 22, 2024

We need to introduce ruff format --check on the pipeline before this. I didn't notice the format was not being enforced.

I'll address the questions you asked on the description soon - it may be faster for me to just apply the answers in code.

Thanks for the PR @jhnstrk . This is really helpful 👍

@Kludex
Copy link
Owner

Kludex commented Jun 12, 2024

We need to add the py.typed file on this PR. I'll review in the next weeks.

@Kludex Kludex mentioned this pull request Jun 12, 2024
@jhnstrk
Copy link
Contributor Author

jhnstrk commented Jun 12, 2024

Thanks for the update.
I added the py.typed file, rebased on the current master (some conflicts) and fixed the formatting flagged by ruff.

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