-
Notifications
You must be signed in to change notification settings - Fork 307
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 support for PEP 561 #618
Changes from 5 commits
818b0c7
a9e2ee6
0d5bcb4
0da4c30
4a32958
ad91821
749e65a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,65 @@ | ||
""" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. By including There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Not quite. These tests result in mypy using the installed After poking around a bit, another option to run mypy on the installed package should be something like like this:
But A workaround to provide confidence that
Note that both of those remove the coverage reports, because they were empty, I'm guessing because the All that said, I think it's clear that |
||
Verify that twine passes type-checking when used as a library. | ||
|
||
https://mypy.readthedocs.io/en/stable/installed_packages.html#making-pep-561-compatible-packages | ||
""" | ||
import subprocess | ||
import textwrap | ||
|
||
import pytest | ||
|
||
|
||
@pytest.fixture | ||
def example_py(tmpdir, monkeypatch): | ||
# Changing directory so that mypy uses installed twine, instead of source | ||
monkeypatch.chdir(tmpdir) | ||
return tmpdir / "example.py" | ||
|
||
|
||
def test_no_missing_import(example_py): | ||
example_py.write("""import twine""") | ||
|
||
subprocess.run(["mypy", "--strict", example_py], check=True) | ||
|
||
|
||
def test_type_error_for_command(example_py): | ||
example_py.write( | ||
textwrap.dedent( | ||
""" | ||
from twine.commands import check | ||
|
||
check.check('example.pkg') | ||
""" | ||
) | ||
) | ||
|
||
with pytest.raises(subprocess.CalledProcessError) as excinfo: | ||
subprocess.run( | ||
["mypy", "--strict", example_py], check=True, stdout=subprocess.PIPE | ||
) | ||
|
||
assert b"1 error in 1 file" in excinfo.value.output | ||
assert ( | ||
b'"check" has incompatible type "str"; expected "List[str]"' | ||
in excinfo.value.output | ||
) | ||
|
||
|
||
def test_type_error_for_class(example_py): | ||
example_py.write( | ||
textwrap.dedent( | ||
""" | ||
from twine import repository | ||
|
||
repo = repository.Repository() | ||
""" | ||
) | ||
) | ||
|
||
with pytest.raises(subprocess.CalledProcessError) as excinfo: | ||
subprocess.run( | ||
["mypy", "--strict", example_py], check=True, stdout=subprocess.PIPE | ||
) | ||
|
||
assert b"1 error in 1 file" in excinfo.value.output | ||
assert b'Too few arguments for "Repository"' in excinfo.value.output |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.