-
Notifications
You must be signed in to change notification settings - Fork 3.2k
[typing] Improve the typing of the with_cleanup decorator #13621
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
base: main
Are you sure you want to change the base?
Conversation
…load, install, lock, and wheel commands
self: _CommandT, | ||
options: Values, | ||
args: list[str], | ||
) -> int: |
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.
Note that there is no use of the | None
, and in all cases the args is list[str]
:
pip/src/pip/_internal/commands/lock.py
Line 92 in 758a172
def run(self, options: Values, args: list[str]) -> int: def run(self, options: Values, args: list[str]) -> int: pip/src/pip/_internal/commands/install.py
Line 281 in 758a172
def run(self, options: Values, args: list[str]) -> int: pip/src/pip/_internal/commands/wheel.py
Line 103 in 758a172
def run(self, options: Values, args: list[str]) -> int:
Hi @pelson, thanks a lot for contributing to pip! |
src/pip/_internal/cli/req_command.py
Outdated
) | ||
|
||
|
||
_CommandT = TypeVar("_CommandT", bound=RequirementCommand) |
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.
Aren't TypeVars usually defined shortly before they're first used or shortly after the imports or even in the if TYPE_CHECKING
block?
Regardless, please move it somewhere other than the very end of the module.
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.
Mechanically this is done because with_cleanup
depends on RequirementCommand
, but that is defined afterwards, so I was forced to make this typevar here.
I am fine to move the with_cleanup
definition to after RequirementCommand
if this is what you'd like me to do.
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.
IIRC mypy will tolerate a forward reference in a typevar bound just fine.
from typing import TypeVar
T = TypeVar("T", bound="Base")
def do_something(obj: T) -> T:
return obj
class Base:
pass
class Wrong:
pass
do_something(Base()) # passes
do_something(Wrong()) # E: Value of type variable "T" of "do_something" cannot be "Wrong" [type-var]
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.
Confirmed. Updated.
…ate bound type (via string)
Improve the typing of the
with_cleanup
decorator, as used by thedownload
,install
,lock
, andwheel
commands.There is zero functional change in this PR, and the typing usage is functional back to Python 3.7 (at least), so should be no compatibility issues.
Originally identified as a result of #13599.