Skip to content
New issue

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

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

Already on GitHub? # to your account

Add mypy strict typing #140

Merged
merged 6 commits into from
Sep 29, 2024
Merged

Add mypy strict typing #140

merged 6 commits into from
Sep 29, 2024

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.

@Kludex Kludex merged commit a169d93 into Kludex:master Sep 29, 2024
6 checks passed
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants