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 type checking #269

Merged
merged 1 commit into from
Jul 1, 2021
Merged

Add mypy type checking #269

merged 1 commit into from
Jul 1, 2021

Conversation

jdufresne
Copy link
Member

@jdufresne jdufresne commented Jan 3, 2021

Type checking helps build confidence that an internally consistent API
is used correctly.

Use type comments rather than annotations to retain compatibility with
Python 2.7.

The functools.cached_property is now used on Python 3.8+ as this helps
mypy understand the type of the cached property.

Rationale

The pip project vendors a copy of distro.py:
https://github.com/pypa/pip/blob/20.3.3/src/pip/_vendor/distro.py

pip also uses mypy. Right now, pip must workaround or ignore type errors
that occur from calls to distro.py. By including types directly in the
library, it will help distro, pip, and any other library users running
mypy.

Copy link
Contributor

@funkyfuture funkyfuture left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i find these changes very helpful and well done.

distro.py Outdated
@@ -37,6 +37,40 @@
import argparse
import subprocess

if False:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what's the rationale for this condition? is that to work with Python 2, where no typing.TYPE_CHECKING is available? if so, a short comment would help in the long term for other readers.

amending # pragma: nocover to this line should calm down that automated commenter here.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the review @funkyfuture. I have applied your suggestions to the latest revision.

Copy link
Contributor

@sethmlarson sethmlarson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The change looks good, just need to hook it up to GHA (and one more comment too)

.travis.yml Outdated Show resolved Hide resolved
distro.py Show resolved Hide resolved
Type checking helps build confidence that an internally consistent API
is used correctly.

Use type comments rather than annotations to retain compatibility with
Python 2.7.

The functools.cached_property is now used on Python 3.8+ as this helps
mypy understand the type of the cached property.

Rationale
---------

The pip project vendors a copy of distro.py:
https://github.com/pypa/pip/blob/20.3.3/src/pip/_vendor/distro.py

pip also uses mypy. Right, pip must workaround or ignore type errors
that occur from calls to distro.py. By including types directly in the
library, it will help distro, pip, and any other library users running
mypy.
Copy link
Contributor

@sethmlarson sethmlarson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thank you!

@sethmlarson sethmlarson merged commit 20cb68d into python-distro:master Jul 1, 2021
@jdufresne jdufresne deleted the mypy branch July 1, 2021 15:09
@hartwork hartwork added this to the 1.6.0 milestone Jul 10, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants