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 more type annotations and improve MonkeyType tooling #600

Merged
merged 19 commits into from
Apr 25, 2020

Conversation

bhrutledge
Copy link
Contributor

@bhrutledge bhrutledge commented Apr 21, 2020

Inspired by @deveshks's interest in helping #231, I wanted to go through the process I had used in #469 and #519, with the goal of documenting it so that other contributors could help get type coverage across the codebase.

I left my commit history intact in case folks are curious, but I'm happy to squash & merge this.

Changes:

  • Add a monkeytype tox environment for inferring type annotations from the test suite
  • Add a "strict" section to the mypy configuration for a subset of modules
  • Add missing type annotations to auth, cli, package, repository, settings, and utils

If folks are good with this, I'll follow up by adding a roadmap in a comment on #231, linked from the original description, including the recipe below. At a high level, I think we should finish the top-level modules, before proceeding to the commands modules. I think it's also worth checking for test coverage, because that can make the inference more accurate.

Recipe for using tox -e monkeytype

Run the test suite under MonkeyType tracing:

$ tox -e monkeytype
  ...
====================== 109 passed, 9 deselected in 3.52s =======================

Apply the inferred annotations to a module, format the changes, and commit:

$ tox -e monkeytype apply twine.wheel
$ tox -e format
$ git commit -am "Apply monkeytype to twine.wheel"

Add the module to the strict section of mypy.ini:

[mypy-twine.auth,twine.cli,twine.package,twine.repository,twine.utils,twine.wheel]

Review the changes and cleanup as necessary. Look out for:

  • Unused imports
  • Complex Unions
  • pretend.stub instead of a real type
  • Errors from tox -e typing
  • Linting and test failures

Commit, then open a PR:

$ git commit -am "Clean up after monkeytype"

@bhrutledge
Copy link
Contributor Author

@pypa/twine-maintainers I hope you'll pardon the ping, but I'm hoping to merge this by Friday. Beyond just keeping the momentum, the discussion in #593 has me thinking that it'd be nice to have the typing working done soon, to strengthen the foundation when adding features. With recipe outlined in the description, I think it's possible that it could be done within the next week.

Of course, happy to hear questions/critiques.

Comment on lines +15 to +18
from typing import Any
from typing import Dict
from typing import List
from typing import Tuple
Copy link
Member

Choose a reason for hiding this comment

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

Definitely a nit, but is there a reason why we're importing like this and not:

Suggested change
from typing import Any
from typing import Dict
from typing import List
from typing import Tuple
from typing import Any, Dict, List, Tuple

Copy link
Member

Choose a reason for hiding this comment

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

Also, would be nice to avoid these imports at runtime, see what the packaging library is doing here: https://github.com/pypa/packaging/blob/61672bf9f507f38e84ce2786a1c42f55fa0a3153/packaging/_typing.py

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Definitely a nit, but is there a reason why we're importing like this

That's a side-effect of:

twine/.isort.cfg

Lines 2 to 3 in 2c30b1e

# NOTE: Consider single_line_exclusions=typing in next version of isort
force_single_line=True

which was added in #576. Still waiting on an isort release that includes PyCQA/isort#1085.

would be nice to avoid these imports at runtime

I've seen the if TYPE_CHECKING pattern before, but never investigated why it was useful. From the packaging link (emphasis mine):

Generally, typing would be imported at runtime and used in that fashion -
it acts as a no-op at runtime and does not have any run-time overhead by
design. As it turns out, typing uses separate sources for Python 2/Python 3. To work around this, mypy allows the typing import to be behind a False-y optional.

My read on this is that if TYPE_CHECKING is useful for Py2/3 compatibility, which Twine doesn't need. The mypy docs also include examples of using it for troubleshooting, e.g. import cycles.

So, I'm happy to add it if it's valuable, but it's not clear to me how it would be.

Copy link
Contributor

Choose a reason for hiding this comment

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

Okay, so if I understand this correctly, as per the google style guide , we are allowed to import multiple specific classes on one line from the typing module.

But there is currently no support from isort in the version we are using to globally force single line imports, but exclude certain modules from that rule, which is what single_line_exclusions=typing would do in a newer version of isort once PyCQA/isort#1085 is merged

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@deveshks That's correct.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@di I'm going to merge this as-is, but I'd love to hear your thoughts about TYPE_CHECKING.

Also: I really enjoyed your Static Typing talk.

@jaraco
Copy link
Member

jaraco commented Apr 23, 2020

No objections.

@deveshks
Copy link
Contributor

Thanks for this @bhrutledge . Once this gets merged, I will try to run some of the new unit tests I ran against it, and also try my hand in adding type annotations with some of the files in twine/commands

@bhrutledge
Copy link
Contributor Author

Once this gets merged, I will try to run some of the new unit tests I ran against it, and also try my hand in adding type annotations with some of the files in twine/commands

@deveshks Thanks for offering to help. Once this is merged, I'll write up a comment in #231 that has a more detailed roadmap, but in short, I'd suggest starting by adding annotations to twine.wheel and twine.wininst using the recipe above (esp. since you wrote the tests). Because those are simpler modules, I think that will be an easier way to learn the tooling. Also, because they're utility modules, adding type coverage will improve the coverage of the modules that import them.

@deveshks
Copy link
Contributor

I'd suggest starting by adding annotations to twine.wheel and twine.wininst using the recipe above (esp. since you wrote the tests).

Agreed, I will start with those, although I haven't written unit tests for twine.wininst. Even though they should be similar to twine.wheel, I am not sure if I can use an exe file in my Mac OSX and try to write those out.

Is there a workaround to do that (Writing and testing windows based tests on a non-Windows PC?)

@bhrutledge bhrutledge merged commit 8765d14 into pypa:master Apr 25, 2020
@bhrutledge bhrutledge deleted the 231-monkeytype-process branch April 25, 2020 10:07
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.

5 participants