Skip to content

test: update mypy typings #956

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

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions invoke/collection.py
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import copy
from types import ModuleType
from typing import Any, Callable, Dict, List, Optional, Tuple
from typing import Any, Callable, Dict, List, Optional, Tuple, Union

from .util import Lexicon, helpline

Expand Down Expand Up @@ -284,7 +284,7 @@ def add_task(

def add_collection(
self,
coll: "Collection",
coll: Union["Collection", ModuleType],
name: Optional[str] = None,
default: Optional[bool] = None,
) -> None:
Expand Down
2 changes: 1 addition & 1 deletion invoke/context.py
Original file line number Diff line number Diff line change
Expand Up @@ -389,7 +389,7 @@ def cd(self, path: Union[PathLike, str]) -> Generator[None, None, None]:
(such as the various ``Path`` objects out there), and not just
string literals.
"""
path = str(path)
path = os.fspath(path)
self.command_cwds.append(path)
try:
yield
Expand Down
2 changes: 1 addition & 1 deletion invoke/parser/context.py
Original file line number Diff line number Diff line change
Expand Up @@ -91,7 +91,7 @@ def __init__(
self.args = Lexicon()
self.positional_args: List[Argument] = []
self.flags = Lexicon()
self.inverse_flags: Dict[str, str] = {} # No need for Lexicon here
self.inverse_flags: Dict[str, Any] = {} # No need for Lexicon here
self.name = name
self.aliases = aliases
for arg in args:
Expand Down
16 changes: 15 additions & 1 deletion invoke/runners.py
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
Optional,
Tuple,
Type,
overload,
)

# Import some platform-specific things at top level so they can be mocked for
Expand Down Expand Up @@ -122,6 +123,16 @@ def __init__(self, context: "Context") -> None:
self._asynchronous = False
self._disowned = False

@overload
def run(self, command: str, *, disowned: None, **kwargs: Any) -> "Result":
...

@overload
def run(
self, command: str, *, disowned: bool, **kwargs: Any
) -> Optional["Result"]:
...

def run(self, command: str, **kwargs: Any) -> Optional["Result"]:
"""
Execute ``command``, returning an instance of `Result` once complete.
Expand Down Expand Up @@ -1481,7 +1492,7 @@ def __init__(
exited: int = 0,
pty: bool = False,
hide: Tuple[str, ...] = tuple(),
):
) -> None:
self.stdout = stdout
self.stderr = stderr
if encoding is None:
Expand All @@ -1506,6 +1517,9 @@ def return_code(self) -> int:
def __bool__(self) -> bool:
return self.ok

def __int__(self) -> int:
return self.exited

def __str__(self) -> str:
if self.exited is not None:
desc = "Command exited with status {}.".format(self.exited)
Expand Down
5 changes: 3 additions & 2 deletions invoke/tasks.py
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
Type,
TypeVar,
Union,
cast,
)

from .context import Context
Expand Down Expand Up @@ -286,7 +287,7 @@ def get_arguments(
return args


def task(*args: Any, **kwargs: Any) -> Callable:
def task(*args: Any, **kwargs: Any) -> "Task[T]":
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The TypeVar is not being used correctly in this function. Either use Task[Any], or if you want to spend a bit more time on it, you could try and type it properly.

I think the correct signature would look something along the lines of:

@overload
def task(arg: T, *, klass: Callable[[T]], U], **kwargs: Any) -> U:
    ...
@overload
def task(arg: T, **kwargs: Any) -> Task[T]:
    ...
@overload
def task(*args, klass: Callable[[Callable], T], **kwargs: Any) -> Callable[[Callable], T]:
    ...
@overload
def task(*args: Any, **kwargs: Any) -> Callable[[T], Task[T]]:  # T should actually be bound to Callable on this one.
    ...

But, frankly, it's rather complicated.

Copy link
Contributor Author

@kuwv kuwv Sep 13, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is following the recommendation provided by mypy documentation: https://mypy.readthedocs.io/en/stable/generics.html#declaring-decorators

Commented on the wrong example.

Copy link
Contributor

@Dreamsorcerer Dreamsorcerer Sep 14, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The mistake is that a TypeVar is being used as a return type, but does not appear in the parameters (or anywhere else), which means that it is not doing the job of a TypeVar. The point of a TypeVar is to infer the precise type based on context. e.g. def foo(a: T) -> T: means that the return type will be the same as the input type. If you only put it in one place, then this gives no information at all (it wouldn't surprise me if mypy were to produce a type error in future, due to this being obviously incorrect usage).

Copy link
Contributor

@Dreamsorcerer Dreamsorcerer Sep 15, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems that mypy already produces an error for this, but it doesn't trigger with a Generic:
python/mypy#16113

"""
Marks wrapped callable object as a valid Invoke task.

Expand Down Expand Up @@ -357,7 +358,7 @@ def inner(body: Callable) -> Task[T]:
return _task

# update_wrapper(inner, klass)
return inner
return cast('Task["T"]', inner)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe this'll be fixed by the above comment, but if not, it'd be better to type ignore than cast here, given that it is literally not a Task being returned.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is following the recommendation provided by mypy documentation: https://mypy.readthedocs.io/en/stable/generics.html#declaring-decorators

Copy link
Contributor

@Dreamsorcerer Dreamsorcerer Sep 25, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My point was that this is not a Task. The docs you point to are accepting a function and returning a function, the cast is to the function type. This is returning a function and then you're casting it as a Task.

docs:

F = TypeVar('F', bound=Callable[..., Any])
def printing_decorator(func: F) -> F:
    def wrapper(*args, **kwds):
        ...
    return cast(F, wrapper)  # wrapper is the same as F as it is a function which passes through the call.

here:

def task(*args: Any, **kwargs: Any) -> Task[T]:
    def inner(body: Callable) -> Task[T]:
        ...
    return cast(Task[T], inner)  # inner is clearly not a Task



class Call:
Expand Down
2 changes: 1 addition & 1 deletion tasks.py
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ def test(

# TODO: replace with invocations' once the "call truly local tester" problem is
# solved (see other TODOs). For now this is just a copy/paste/modify.
@task(help=test.help) # type: ignore
@task(help=test.help)
def integration(
c: "Context", opts: Optional[str] = None, pty: bool = True
) -> None:
Expand Down
2 changes: 1 addition & 1 deletion tests/context.py
Original file line number Diff line number Diff line change
Expand Up @@ -213,7 +213,7 @@ class Path:
def __init__(self, value):
self.value = value

def __str__(self):
def __fspath__(self):
return self.value

runner = Local.return_value
Expand Down