Skip to content

Add type annotations#200

Merged
di merged 12 commits intopypa:masterfrom
di:fix/189
Sep 25, 2019
Merged

Add type annotations#200
di merged 12 commits intopypa:masterfrom
di:fix/189

Conversation

@di
Copy link
Copy Markdown
Member

@di di commented Sep 14, 2019

Fixes #189. Adds Python-2 compatible type annotations.

(cc @pradyunsg @cjerdonek)

@di di requested a review from brettcannon September 14, 2019 19:50
Copy link
Copy Markdown
Member

@pradyunsg pradyunsg left a comment

Choose a reason for hiding this comment

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

THANK YOU for doing this! This looks great!

A bunch of nit-picks (please do feel free to ignore them/mark them as resolved if you don't think they add value) and a couple of minor concerns around how we're handling some of mypy's weirdness.

Comment thread packaging/markers.py
Comment thread packaging/specifiers.py
Comment thread packaging/specifiers.py Outdated
Comment thread stubs/platform.pyi Outdated
Comment thread tox.ini Outdated
Comment thread packaging/tags.py Outdated
Comment thread packaging/tags.py Outdated
@brettcannon
Copy link
Copy Markdown
Member

FYI I do plan to review this, but I'm waiting until #199 lands and I get 19.2 released.

@di
Copy link
Copy Markdown
Member Author

di commented Sep 19, 2019

@brettcannon This is ready for you to review.

Copy link
Copy Markdown
Member

@brettcannon brettcannon left a comment

Choose a reason for hiding this comment

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

Thanks for doing this! There's just a couple of places where the types are too strict for the input.

Comment thread packaging/_structures.py
Comment thread packaging/markers.py Outdated
Comment thread packaging/tags.py Outdated
Comment thread packaging/tags.py
Comment thread packaging/tags.py Outdated
Comment thread packaging/tags.py Outdated
Comment thread packaging/tags.py Outdated
Comment thread packaging/tags.py Outdated
Comment thread packaging/tags.py Outdated
@brettcannon
Copy link
Copy Markdown
Member

Once this PR lands we should open an issue for setting up a mypy.ini file so that code editors can run mypy on the user's behalf and have the appropriate flags triggering (and I'm happy to put the time in to create the file). But until then the tox solution seems fine to me.

@di
Copy link
Copy Markdown
Member Author

di commented Sep 23, 2019

Once this PR lands we should open an issue for setting up a mypy.ini file so that code editors can run mypy on the user's behalf and have the appropriate flags triggering (and I'm happy to put the time in to create the file). But until then the tox solution seems fine to me.

These settings are in the setup.cfg file (because it already existed), which is a valid fallback configuration file for mypy. Maybe this is a bug in your editor instead? 😉

@di di requested a review from brettcannon September 23, 2019 19:14
@brettcannon
Copy link
Copy Markdown
Member

@di Sorry, still a little sick so I somehow marked setup.cfg as reviewed without reviewing it. 😄

Comment thread setup.cfg
# The following are the flags enabled by --strict
#
# Note: warn_unused_ignores is False due to incorrect typeshed annotations for
# platform.mac_ver()
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Is there a bug upstream tracking this?

@brettcannon
Copy link
Copy Markdown
Member

@di can you do the merge yourself or do you need one of us to do it?

@di di merged commit afc5847 into pypa:master Sep 25, 2019
@di di deleted the fix/189 branch September 25, 2019 18:06
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.

Add type annotations to packaging.tags

4 participants