-
Notifications
You must be signed in to change notification settings - Fork 253
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
Refactor type hints #754
Refactor type hints #754
Conversation
f60d535
to
d75c57e
Compare
Could you please mention what these changes / PRs would be doing, and how they'd reduce the import overhead? |
The majority of the improvement will be lazily loading imports such as subprocess and logging, and I may defer regular expression compilation in a second PR however I don't see that being a huge improvement. |
|
This PR is ready for review! |
Why are these changes needed for ofek#1? |
I thought I would do a refactor first since I saw a performance benefit to separating type definitions into a separate module. The benefit turned out to be nonexistent because the annotations import has an equivalent cost (it will go away eventually) but I figured this is useful anyway since there is no benefit to defining runtime types. I think the changes here are useful as the typing people are encouraging the more built-in approaches and heavily discouraging (I can't find where at the moment) the use of Optional and Union. |
# MarkerAtom = Union[MarkerItem, List["MarkerAtom"]] | ||
# MarkerList = List[Union["MarkerList", MarkerAtom, str]] | ||
# mypy does not support recursive type definition | ||
# https://github.com/python/mypy/issues/731 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this is valid anymore. The issue is closed in the mypy repo, maybe you could check? WOuld be nice to avoid hauling around the tech debt.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought the same thing and tried but encountered errors.
I came here to merge this and realised that the codebase has evolved meaningfully since this PR was filed, so filed an updated #785. |
After that is merged I will do the lazy import performance improvement in a separate PR! |
This PR uses
from __future__ import annotations
to allow for:Dict[str, List[str]]
->dict[str, list[str]]
Optional
in favor of| None
e.g.str | None
Union
in favor of|
e.g.str | int
Notes:
VersionComparisonMethod
type alias insrc/packaging/version.py
was unused so I removed itsrc/packaging/_types.py
but type definitions must use the old-style type classes because it is ambiguous that they are type aliases for use in annotations rather than assignments and typing.TypeAlias was introduced only very recently in 3.10packaging
modules