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

Type annotations #972

Closed
16 tasks done
atugushev opened this issue Oct 26, 2019 · 22 comments · Fixed by #1310 or #1350
Closed
16 tasks done

Type annotations #972

atugushev opened this issue Oct 26, 2019 · 22 comments · Fixed by #1310 or #1350
Labels
enhancement Improvements to functionality help wanted Request help from the community

Comments

@atugushev
Copy link
Member

atugushev commented Oct 26, 2019

What's the problem this feature will solve?

It would be nice to have type annotations throughout the codebase. This issue is a general tracking issue of how we would use mypy and typings.

Describe the solution you'd like

  • use pytest-annotate to auto add typings (or something similar)
  • add typings incrementally, one PR per module (e.g., 21b7130) to help reviewers

Alternative Solutions

Wait until we drop Python 2 support and add Python 3 style type annotations.

Additional context

Docs:

Useful tools:

Progress

Maintenanse
Add typings to the following files
Final steps

This list must be done after the above lists.

@atugushev atugushev added the maintenance Related to maintenance processes label Oct 26, 2019
@atugushev atugushev changed the title Type annotations? Type annotations Oct 26, 2019
@atugushev
Copy link
Member Author

Hey, @jazzband/pip-tools. Please, join the discussion. Have you any other thoughts or helpful tools we could use to integrate type annotations?

@atugushev atugushev added the help wanted Request help from the community label Oct 26, 2019
@webknjaz
Copy link
Member

Could probably use https://github.com/Instagram/MonkeyType to collect types from runtime.

P.S. I think, it's better to use a separate mypy.ini rather than flooding setup.cfg with things that aren't related to packaging.

@codingjoe
Copy link
Member

@atugushev I don't want to discourage you, but I believe effort and return might be a bit out of balance. I don't think there are too many people that use the pip-tools Python API. For most people this is a nice CLI tool. The only people who would benefit from type hints however are people who use the Python API are developers of this package. That being said, good IDEs like PyCharm will give you type hints, whether they are explicit added or not. If this is only about you wanting type hints to develop pip-tools and for some reasons don't have a PyCharm license, you can always apply for an open source license ;)

I'm really just dreading the immense workload this will create especially for the poor soul reviewing it :P

P.S. I think, it's better to use a separate mypy.ini rather than flooding setup.cfg with things that aren't related to packaging.

@webknjaz I think, I actually prefer having everything in the setup.cfg. For me it makes packaging easier, since I don't have to prune too many files from dists. That being said, there are also a bunch of tools like Sphinx and the PyTest runner that encourage you to use the setup.cfg, since both command are actually run via setup.py.
Then again, it does boil down to taste. I'm am just saying there are may flavors ;)

@atugushev
Copy link
Member Author

@webknjaz

Could probably use https://github.com/Instagram/MonkeyType to collect types from runtime.

AFAIK, MonkeyType doesn't support py2 typings comments so that we can afford for now py3 typings.

P.S. I think, it's better to use a separate mypy.ini rather than flooding setup.cfg with things that aren't related to packaging.

Personally, I'd prefer to have the tools configs in the setup.cfg, and I'm quite sure we had discussed it earlier with members (can't find references though, probably with @blueyed?) to collect all configurations there. But I understand your intention, and it seems pretty reasonable. Would you like to file a separate issue so we can discuss it?

@atugushev
Copy link
Member Author

@codingjoe

@atugushev I don't want to discourage you, but I believe effort and return might be a bit out of balance. I don't think there are too many people that use the pip-tools Python API. For most people this is a nice CLI tool. The only people who would benefit from type hints however are people who use the Python API are developers of this package. That being said, good IDEs like PyCharm will give you type hints, whether they are explicit added or not. If this is only about you wanting type hints to develop pip-tools and for some reasons don't have a PyCharm license, you can always apply for an open source license ;)

Actually, I do have PyCharm open source license, and I'm happy to have this opportunity!

My thoughts on typings:

  1. it gives more context to the function/method signature and would help contributors to understand the code
  2. mypy is an extra eye on the code quality, it really helps to find bugs sometimes
  3. typings makes the code more strict

IMO it's worth it. The codebase is not so large (comparing to pip), so it's not going to be too hard to adopt typings, thanks to auto type tools!

@aleksihakli
Copy link
Member

Just my 2c: adding explicit typing to the codebase is not that big of a job and definitely makes the code more understandable and maintainable :)

@codingjoe
Copy link
Member

OK if it helps people, sure, I'm not against it.

@sfdye sfdye added the enhancement Improvements to functionality label Mar 21, 2020
@sfdye
Copy link
Member

sfdye commented Mar 21, 2020

Alternative Solutions
Wait until we drop Python 2 support and add Python 3 style type annotation

TBH I'd prefer this approach

So my question is when are we gonna drop Python 2 support? the discussion on Packaging and Python 2 is so long that I don't have the time to go through them all 😛

But it's March 2020 now, 3 months into 💀 of Python 2.7.

@atugushev
Copy link
Member Author

@sfdye

So my question is when are we gonna drop Python 2 support?

As soon as pip does.

@codingjoe
Copy link
Member

codingjoe commented Mar 25, 2020

As soon as pip does.

So never 😉

@atugushev
Copy link
Member Author

@codingjoe

So never 😉

BTW, pip==21.0 (Jan 2021) will be the last version with python 2.7 support, see pypa/pip#8087.

@hugovk
Copy link
Member

hugovk commented Apr 20, 2020

BTW, pip==21.0 (Jan 2021) will be the last version with python 2.7 support, see pypa/pip#8087.

Correction: pip 21.0 (Jan 2021) will be the first version to drop support for Python 2.7.

pip 20.3 (October 2020) will be the last version to support Python 2.7.

pypa/pip#6148 (comment)

@codingjoe
Copy link
Member

Yes, finally. I sure you can tell, but boy am I looking forward to Python 2 dying ☠️

@pradyunsg
Copy link
Contributor

Why not add # type: comments?

@codingjoe
Copy link
Member

Why not add # type: comments?

I guess there are many ways to add type hints via doc strings. Sphinx annotation (including napoleon) have been around for a while and are supported by all good IDEs.

I would probably go with Google Style annotations, since they are human and machine-readable. After all, we make documentation for humans not machines. That being said, I don't see a huge advantage of using adding type hints. This isn't a package that a lot of people interact with on a Python API level, but rather via CLI. Therefore, I am only in favor of adding type hints, if they do not slow down regular development or otherwise introduce complexity.

@atugushev atugushev removed the maintenance Related to maintenance processes label Apr 21, 2020
@atugushev
Copy link
Member Author

I'd prefer to wait with patience until pip drops Python 2.7 support and then we can use type hinting with a full power!

@pradyunsg
Copy link
Contributor

I guess there are many ways to add type hints via doc strings. Sphinx annotation (including napoleon) have been around for a while and are supported by all good IDEs.

Except, those aren't supported by mypy or pytype; neither are they "blessed" by a PEP describing them. :)

Anyway, @atugushev's approach also works well.

@sfdye
Copy link
Member

sfdye commented May 15, 2020

So that's 2021 at least

@jdufresne
Copy link
Member

jdufresne commented Dec 16, 2020

Given that we expect pip to drop Python 2 support in the new year, what do you think about cutting one last pip-tools release and then we continue working towards type hinting the code base today? There is already a PR out there to drop Python 2 #1243.

Doing it now would allow people with some extra free time over the holidays to get some of this work done.

WDYT?

My motivation for including type checking is that it helps provide additional confidence that APIs are being used correctly (both internal and external) during large upgrades and refactorings. I'm happy to lend a hand in adding types to the code incrementally.

@codingjoe
Copy link
Member

I would also be for one final Python 2 release and then to drop support it like a hot potato.
Gradual changes seem most practical, no one wants to review and release a 10kloc patch.

@atugushev
Copy link
Member Author

atugushev commented Dec 30, 2020

I'm going to prepare the 5.5.0 release soon and then we can drop 2.7/3.5 and finally add type hints!

jdufresne added a commit that referenced this issue Jan 2, 2021
Type checking helps give confidence that APIs are being used correctly,
especially during large refactoring (e.g. eventually dropping Python 2
support).

This initial pass only includes type annotations that were necessary to
make mypy pass. Future pull requests will add types for the rest of the
project once the workflow is established.

pip has started including type annotation in a subset of its code. When
major changes to pip are released, the type checking will help identify
API mismatches and areas to adjust.

mypy is configured to be as strict as possible without introducing
errors. The configuration is a subset of the --strict CLI argument.

Refs #972
@atugushev
Copy link
Member Author

atugushev commented Feb 4, 2021

Started to work on type hints #1310. Any help with review or code is appreciated ❤️

@codingjoe codingjoe linked a pull request Feb 5, 2021 that will close this issue
@atugushev atugushev reopened this Feb 6, 2021
jdufresne added a commit that referenced this issue Feb 8, 2021
This option is part of the set of "strict" mypy options, bringing the
project closer to full strict checking.

https://mypy.readthedocs.io/en/stable/command_line.html#cmdoption-mypy-no-implicit-reexport

> By default, imported values to a module are treated as exported and
> mypy allows other modules to import them. This flag changes the
> behavior to not re-export unless the item is imported using from-as or
> is included in `__all__`.

A side benefit of using `__all__` is that it allows removing the flake8
exceptions from the code base. Now `__init__.py` files are checked too.

Refs: #972
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Improvements to functionality help wanted Request help from the community
Projects
None yet
8 participants