search

pyca / pynacl

Python binding to the Networking and Cryptography (NaCl) library
https://pynacl.readthedocs.io/
Apache License 2.0
1.06k stars 233 forks source link

Add mypy to CI #693

Closed DMRobertson closed 2 years ago

DMRobertson commented 2 years ago

Following the strategy outlined in https://github.com/pyca/pynacl/pull/692#issuecomment-948201912, I've

DMRobertson commented 2 years ago

Not sure why the pypy CI build failed. Looks like it couldn't build typed-ast from source because codecs.h wasn't on the C include path?

Dreamsorcerer commented 2 years ago

Not sure why the pypy CI build failed. Looks like it couldn't build typed-ast from source because codecs.h wasn't on the C include path?

Looks like there was a related issue on aiohttp, but I don't know the details: https://github.com/aio-libs/aiohttp/pull/5496

DMRobertson commented 2 years ago

Looks like there was a related issue on aiohttp, but I don't know the details: aio-libs/aiohttp#5496

Seems like the underlying cause is https://github.com/python/typed_ast/issues/111 . One of the linked PRs was https://github.com/nolar/kopf/pull/845 , which decided to only run mypy under CPython. So maybe it's best to pull mypy out from the meta job and into its own job, and then configure GHA to only run that under CPython.

reaperhulk commented 2 years ago

I would prefer that this be structured like the mypy job on pyca/cryptography (which adds a new mypy toxenv and only installs the deps there). This avoids the pypy issue as well and gives us some consistency across the projects (which is useful for me!).

Other than that this looks good, although I'd also like to get the tests repo into the mypy job as quickly as we can since obviously this doesn't do much yet. Subsequent PRs can take care of that as we land chunks of the larger work!

Thanks for all the work you're doing here; this will be a great addition for the next release.

DMRobertson commented 2 years ago

@reaperhulk Many thanks, I'll try to get that done soon. But to double check:

I would prefer that this be structured like the mypy job on pyca/cryptography (which adds a new mypy toxenv and only installs the deps there). This avoids the pypy issue as well and gives us some consistency across the projects (which is useful for me!).

Were you thinking of pyca/bcrypt here? AFAICS the pyca/cryptography tox configuration lumps mypy in to the flake env.

reaperhulk commented 2 years ago

Thanks for working with me on this! I look forward to the next PR 😄 If at all possible it'd be great to split this into as many PRs as is reasonably feasible.

DMRobertson commented 2 years ago

Thanks for working with me on this! I look forward to the next PR smile If at all possible it'd be great to split this into as many PRs as is reasonably feasible.

My pleasure: thank you for having me. My plan is to tackle each module in a PR by itself, starting with #694.