Add support for PEP 561 #618
Conversation
Indicating that twine includes type information, when used as a library https://mypy.readthedocs.io/en/stable/installed_packages.html#making-pep-561-compatible-packages
tests/test_mypy_integration.py
Outdated
| @@ -0,0 +1,65 @@ | |||
| """ | |||
There was a problem hiding this comment.
By including integration in the filename, this can be skipped as suggested in #543 (comment).
There was a problem hiding this comment.
Also, I wrote these tests as something of an exercise. I'm not sure how much value they provide going forward, so I'm happy to remove them if they feel noisy.
There was a problem hiding this comment.
To be clear, these tests verify what running mypy against our repository verifies, right? We use twine as a library in the commands
There was a problem hiding this comment.
These tests verify what running mypy against our repository verifies, right?
Not quite. These tests result in mypy using the installed twine package, instead of the twine/ source. Without py.typed, they fail.
After poking around a bit, another option to run mypy on the installed package should be something like like this:
[testenv:types]
deps =
mypy
changedir = {envtmpdir}
commands =
mypy --config-file {toxinidir}/mypy.ini --package twine
But --package is a bit broken: python/mypy#7087.
A workaround to provide confidence that py.typed is working as expected would be something like:
commands =
mypy --config-file {toxinidir}/mypy.ini {toxinidir}/twine
mypy --command "import twine"
Note that both of those remove the coverage reports, because they were empty, I'm guessing because the changedir prevents mypy from finding the source.
All that said, I think it's clear that py.typed is working as expected. It seems unlikely that there will be a regression, so these tests seem like unnecessary complexity. I'm going to remove them.
| @@ -44,6 +44,7 @@ install_requires= | |||
| keyring >= 15.1 | |||
| setup_requires = | |||
| setuptools_scm >= 1.15 | |||
| zip_safe = False | |||
There was a problem hiding this comment.
The mypy docs (and some closed issues) suggest adding this, but cursory testing suggests it might not be necessary. I opted to be conservative and leave it it.
There was a problem hiding this comment.
Please remove this. It's cruft that I'm trying to discourage users from using. Most packages should aim to be zip safe anyway, and if installed using pip, it won't make a difference. Plus, mypy should support zip-safe packages.
There was a problem hiding this comment.
To get some more clarity, I opened python/mypy#8802. I'm going to wait a day or two to see if anyone chimes in, otherwise I'll happily remove it.
There was a problem hiding this comment.
@jaraco Any thoughts based on the comments in the mypy issue? Based on my limited understanding, I agree that this seems mostly unnecessary, but also maybe mostly harmless?
I'm game to update the mypy docs as suggested (and maybe prevent the further spread of zip_safe = False), but I'd like to get a more concrete understanding first.
There was a problem hiding this comment.
The harm comes from adding unnecessary cruft and creating a model that other users may copy. I'd like this project to be a model of excellence that other projects should feel comfortable copying. I added some comments there, but I'm 99% certain there's no unmitigatable harm in twine omitting this setting.
There was a problem hiding this comment.
BTW, thanks for running this to ground rather than just addressing it here. I know that's a lot of effort... to find the right solution, and I appreciate it very much.
For #231, indicate that twine includes type information, when used as a library:
https://mypy.readthedocs.io/en/stable/installed_packages.html#making-pep-561-compatible-packages
Changes:
twine/py.typedand modifysetup.cfgas suggested by mypy docsI also verified this manually in another project by using
pip install ../twine/dist/twine-3.1.2.dev71+g038ffd1-py3-none-any.whland runningmypy --strict.