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

Merge type annotations from typeshed #382

Open
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

Avasam
Copy link
Contributor

@Avasam Avasam commented Feb 20, 2025

Following the comment #190 (comment) I looked at applying typeshed's boltons stubs inline
This also obsoletes #318

I noticed a few incorrect types in typeshed, so this PR should mirror python/typeshed#13517

The goal of this PR is not to "complete" any type, but simply to apply what's found in typeshed. There's one major distinction where I'm leaving annotations empty in places where typeshed used Incomplete. This doesn't change anything for mypy, but causes pyright to use its special Unknown type.

All _typeshed-imported items could probably go in typeutils.py

Please review carefully, and feel free to ask questions!

By nature of "merging" annotations from another project, it's kindof a big PR (it touches literally all files). I can split this up into individual modules if it would help.

This doesn't yet add mypy/pyright to the CI. A lot would have to be disabled, and some implementations slightly teaked. Given the very dynamic nature of some of these boltons, signatures differing from superclasses, use of sentinel values, etc.
But I can add a very basic config on top of this PR if you'd like. Given the use of tox and pytest I assume you'd prefer to run mypy as a pytest plugin using https://pypi.org/project/pytest-mypy/ ? (note that's not feasible for pyright though)

@Avasam Avasam force-pushed the add-typeshed-types branch 4 times, most recently from 633c8b2 to 1380d5f Compare February 20, 2025 03:28
This was referenced Feb 20, 2025
@mahmoud
Copy link
Owner

mahmoud commented Feb 20, 2025

Hey this looks really great! It'll take me a little while to go through, but it does seem like pulling in typeshed is the best starting point. Great work there.

Regarding testing, I think it's pretty important we add something to get the ball rolling on checking the types, especially for future contributions, even if it's just the expedient pytest-mypy plugin, no matter how basic or incomplete.

As for questions, just one for now: Are there any big leaps you're familiar with between 3.7 and 3.8 that would drastically improve the typing proposition? I'm not against inching up min version support if there's good reason. From my end, in my applications, the biggest leap has been around 3.10, but I think that's a lot of versions to drop at once given boltons's intended conservative backwards compat approach.

@Avasam Avasam force-pushed the add-typeshed-types branch from 1380d5f to 7118058 Compare February 20, 2025 20:44
@Avasam
Copy link
Contributor Author

Avasam commented Feb 20, 2025

Are there any big leaps you're familiar with between 3.7 and 3.8 that would drastically improve the typing proposition?

Not anything that's blocking or that I would personally consider major:

  • Literal and SupportsIndex were added to typing in Python 3.8, so it would reduce usage of typing_extensions (in like 1 file, because everythign else imports Self)
  • collections (list, tuple, dict, set, frozenset and stuff from collections.abc) are subscriptable as of Python 3.9. So that would reduce the need for from __future__ import annotations, and the use of typing imports for subclassing
  • The last version of mypy that can be run on Python 3.7 is https://pypi.org/project/mypy/1.4.1/ (that's kinda bad, but you could always skip type-checking 3.7 on CI if it's an issue)
  • The last version of mypy that can be run on Python 3.8 is https://pypi.org/project/mypy/1.14.1/

Additional notes:

  • Type union operator | is in Python 3.10 anyway. So whether you prefer using | with from __future__ import annotations, using quoted annotations, or typing.Union is mostly stylistic
  • I purposefully put all typing_extensions behind a TYPE_CHECKING check to avoid adding it as a dependency

@Avasam
Copy link
Contributor Author

Avasam commented Feb 21, 2025

And there we go. Mypy installed and running on all appropriate jobs. With a few TODO comments.
And I restored a test.
Btw it spotted that the misc folder hasn't been updated to Python 3-only code.

@mahmoud
Copy link
Owner

mahmoud commented Feb 21, 2025

@Avasam amazing work, and such quick turnaround. I guess I shouldn't be surprised from a speedrunner. Re: the changedir, I see you've restored it, but in case you were still wondering; python adds the current working directory to the path, so it's possible in some cases for boltons to get picked up from the cwd instead of the site-packages version. This affects local more than CI, but can mean you're testing files that aren't making it into the package (and other sneaky failure cases). A more modern mitigation for this is to have a src directory. (I say modern and realize that post is 10 years old. And boltons is older!)

@Avasam
Copy link
Contributor Author

Avasam commented Feb 21, 2025

I realized pretty quick that import boltons in the tests meant it would import from the local folder boltons. So you just needed to be in a different folder. src-layout would work, but that's a non-issue here. I'll leave that to whoever else would like to do those changes for minimal value :P

The restriction to target the virtual environment forced me to learn a bit more about tox and how to run two commands in parallel independant of if one failed. Worked it out by targetting multiple environments on the same invocation. Honestly I prefer this setup anyway over pytest-mypy: to me it always felt like a redundant extra layer that can come with its own issues.
This setup would also work well with pyright (which doesn't have a pytest plugin) and is something I'll keep in mind for other tox-based projects.


About line wrapping: I don't see a linter or formatter config, nor a contributing guide. So I just applied annotations without moving the params or regards for line length. If you have a "rule of thumb" you'd like me to follow I can apply it.


Sidenote about the commit history: I stopped force-pushing in case you started reviewing. But once it's all done I can either rebase it or you can squash-merge.

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.

2 participants