PEP 484 type annotations for Twine

[ygingras]

Update 4/25/2020: See #231 (comment) below for the current roadmap to completion.

I am considering adding type annotations to Twine to help my team do static analysis of code that depends on it. Would a pull request containing type annotations be merge back?

More info on type annotations:
PEP 484: https://www.python.org/dev/peps/pep-0484/
mypy, the type-checker of choice these days: http://mypy-lang.org/

Given the need to support Python 2.x, the preferred format for annotations would be as Type Comments, but I would be open to do .pyi stubs if that makes it easier for everyone.

Please provide guidance and requirements and I will take it from there.

[sigmavirus24]

@ygingras Where to .pyi stubs tend to live? Would it be acceptable to store them in a separate directory from the source, e.g., _stubs?

[ygingras]

Ian, the stubs can be anywhere and a _stubs dir would be perfect for them, indeed.

[sigmavirus24]

Let's do that then. =) I've been interested in them for a while, and this wouldn't be a bad project to experiment with them.

[sigmavirus24]

Hey @ygingras how has this been going?

[brainwane]

Tim Abbott of Zulip has written up a guide to adding static type annotations to a pre-existing codebase that folks might find helpful if they want to start working on adding these annotations to twine:

how mypy works, the benefits and pain points we’ve seen in using mypy, and share a detailed guide for adopting mypy in a large production codebase (including how to find and fix dozens of issues in a large project in the first few days of using mypy!).

[ygingras]

Hi everyone. It was probably obvious from my radio silence, but I have move on to different projects and I won't be doing this work myself. I still think the annotations would be useful, but since this was mostly an idea to scratch one of my itches, I think we can safely close this issue unless someone else is particularly excited about this work.

[brainwane]

@waseem18 would you be interested in working on this with me? It's a bit of a big project but it takes place in several steps and over several PRs I think we could make progress.

[waseem18]

I would absolutely love to work on this @brainwane
I'll start going through the Zulip content, Project documentation and PEP 484.

[fhocutt]

A handful of us at PyCon are interested in working on this. @sigmavirus24 @waseem18 did you have strong preferences for using stub files instead of python2-compatible annotations in the code itself? Personally I prefer not having to switch between files to get the type information but can go with the stubs.

[sigmavirus24]

@fhocutt I would prefer that they be in separate stub files. I would also prefer them to live along side the files they're annotating. I think @waseem18 has started working on that

[waseem18]

@fhocutt Good to hear about folks being interested in this. I have started working on this and have put up an initial pull request #334.

Looking forward to working with you in type hinting Twine. :)

[fhocutt]

Awesome! I see the PR. I'm recovering from PyCon right now and probably won't have time to come back to this till next week sometime. I started annotating utils.py to start getting a feel for mypy. I think that at least to start, we should turn the strictness down from what you have there and ratchet that up once it's starting to come together.

So far I have figured out (among other things) that running the --py2 checks is going to be a little hairy, since it doesn't play nicely with unicode_literals.

[crwilcox]

The existing PR is #344

[bhrutledge]

The existing PR is #344

I'm building on this PR in #466.

[carljm]

It's worth noting that annotations in stub files are purely for the benefit of external users of twine as a library (are there many of those?), and bring zero type safety benefit to twine itself. If there are stub files, mypy won't even look at any of the actual twine code. If anything, stub files bring some maintenance burden as they can easily get out of date with the code, and need to be kept updated with all API changes in twine. This maintenance burden could be worth it if there are lots of people using twine's API as a library who will benefit, but I thought twine was mostly a CLI tool?

Doing annotations in the code itself (as comments, for Py2 support) is really a totally different thing from stub files, as it allows twine's own code to be type checked, and this also prevents the possibility of them getting out of date.

[bhrutledge]

Thanks for the feedback, @carljm. There's a proposal for a "real Twine API" in #361, which I'm guessing is some of the motivation for stub files, but maybe @sigmavirus24 can elaborate.

As a newbie, it does seem easier to start bringing the value of typing to Twine by doing inline annotation. I'm inclined to take #466 in that direction, most likely using the Python 3 syntax since #437 seems likely to be merged.

[bhrutledge]

@theacodes and @jaraco: Would you like me to keep working on this? If so, I'd be inclined to continue with #469 (which depends on the merge of #437). In the case, #466 and #344 could be closed.

[theacodes]

If it's something you want to work on, go for it!

…

On Sun, Jul 7, 2019 at 3:23 AM Brian Rutledge ***@***.***> wrote: @theacodes <https://github.com/theacodes> and @jaraco <https://github.com/jaraco>: Would you like me to keep working on this? If so, I'd be inclined to continue with #469 <#469> (which depends on the merge of #437 <#437>). In the case, #466 <#466> and #344 <#344> could be closed. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#231?email_source=notifications&email_token=AAB5I44UXU24GROSETBDFM3P6G73LA5CNFSM4C7XRRKKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODZLIV6A#issuecomment-508988152>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAB5I46EAESCD6G3GFSQBA3P6G73LANCNFSM4C7XRRKA> .

[bhrutledge]

With the merge of #469, we have a passing mypy check via tox -e lint-mypy. I'm planning to add more type coverage following a similar process as that PR (i.e., using MonkeyType).

I'm ambitiously aiming for a strict check, ala pypa/packaging.

[deveshks]

Are PRs still welcome to add type annotations to different twine modules?

I see there are some of them like twine.commands.check and twine.wheel don't have type annotations added

[sigmavirus24]

Yes @deveshks

[bhrutledge]

@deveshks Yes, though if you don't mind waiting a bit, I've got a process that has worked well for me, that I think will save you time and make PR's easier to submit/review. I'll prioritize running through it again, and document it here.

[deveshks]

Sure thing,

What I was planning to do is to add type annotations one by one, and run mypy <file_name>.py and tox -e typing to check that nothing broke. But I will wait for the process before tackling the PRs

[bhrutledge]

Thanks @deveshks. As a preview, my process has been to use MonkeyType to generate the annotations based on the test suite. As mentioned in that #469, this involves some cleanup, but I found it more straightforward and educational than attempting to do it by hand.

However, with that in mind, working on #7 to add test coverage would benefit this process.

[bhrutledge]

Roadmap to completion

i.e., when the Twine codebase passes with the mypy --strict configuration.

TODO (using the recipe below)

• Annotate wheel, then wininst Add type annotations to twine.wheel and twine.wininst #607
• Annotate exceptions Annotate exceptions, commands, and commands.upload #606
• Annotate commands.__init__ Annotate exceptions, commands, and commands.upload #606
• Annotate commands.upload Annotate exceptions, commands, and commands.upload #606
• Annotate commands.check Add type annotations to twine.commands.check #608
• Annotate commands.register Add type annotations to twine.commands.register #609
• Annotate __main__, __init__, and _installed Use strict mypy settings globally #613
• Make the "strict" settings in mypy.ini global Use strict mypy settings globally #613
• Move mypy.ini to .mypi.ini for consistency with other tooling configuration
  • Not doing; has to be mypy.ini according to the docs
• Review coverage reports from tox -e typing in mypy/
  • There may not be anything actionable here, but I think they're interesting
  • Missing coverage will probably mostly be Any expressions due to untyped third-party packages Add stubs for some missing imports #617
  • Leaving them enabled in tox config
• See if we can avoid the global ignore_missing_imports in mypy.ini Tidy mypy and tox config #614
• Remove [testenv:monkeytype] from tox.ini Tidy mypy and tox config #614
• Indicate PEP 561 compatibility Add support for PEP 561 #618
• Ignore tests Tell mypy to ignore tests #619
• Add mypy to contributing guide Expand "Code style" section in contributing guide #631

Open questions

• Should we add add minimal stubs for packages that are missing them? Add stubs for some missing imports #617
• Should we use if TYPE_CHECKING: around from typing import ... (discussion) ? @di
• Should we prefer more generic annotations? For example:
  • Iterable[str] instead of List[str]
  • Sequence[str] instead of Tuple[str, ...]

References

• Dustin Ingram - Static Typing in Python - PyCon 2020
• Carl Meyer - Type-checked Python in the real world - PyCon 2018
• mypy changelog
• The mypy configuration file
• Using mypy with an existing codebase

Recipe for adding annotations

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

$ git commit -am "Clean up after monkeytype"

Open a PR.

[deveshks]

I am done with wheel and wininst and will raise a PR for the same

I can perhaps take up annotating commands.register as well, along with _main__ and __init__.

[bhrutledge]

That's great @deveshks, but let's do one PR at a time (i.e., justwheel and wininst).

[deveshks]

That's great @deveshks, but let's do one PR at a time (i.e., justwheel and wininst).

Agreed. PR #607 adds type annotations for just wheel and wininst

[deveshks]

Hi @bhrutledge

Please let me know if I can take up some of the remaining 5 tasks in the list to help out with this effort.

[bhrutledge]

@deveshks Thanks again for your enthusiasm, but I don't think there's much parallel work to be done at this point. I've got the changes ready to go for the tasks with my name on them. I think the missing imports and coverage reports are related, and I've already put some work into that, and am keen to see it through.

[bhrutledge]

Just checked off the last item in the roadmap: #231 (comment). So, I think this is done! Any enhancements to or issues with Twine's type checking should be reported as new issues.

Thanks to everyone who contributed along the way, and especially to @deveshks who, in addition to adding type coverage, gave me the nudge I needed to finish this.

[bhrutledge]

Oh, and how could I forget: huge thanks to @brainwane for getting me started on this in the first place, and helping me put together the roadmap to completion.

[brainwane]

Congratulations and thanks!