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

test: update mypy typings #956

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
Open

Conversation

kuwv
Copy link
Contributor

@kuwv kuwv commented Jul 10, 2023

No description provided.

@kuwv
Copy link
Contributor Author

kuwv commented Jul 10, 2023

@bitprophet this should resolve multiple open issues with typing.

@kuwv
Copy link
Contributor Author

kuwv commented Aug 18, 2023

@bitprophet do you think you can review this soon?

It should resolve multiple outstanding issues.

@kuwv
Copy link
Contributor Author

kuwv commented Aug 18, 2023

@Dreamsorcerer fysa

@@ -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

@@ -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

@joaonc
Copy link

joaonc commented Jul 20, 2024

Would love to see this merged. Per https://bitprophet.org/projects

Upcoming
2024.07-8: will likely focus on Invoke features and major bugfixes

So maybe that will happen soon 🤞

@Silverteal

This comment was marked as resolved.

@kuwv
Copy link
Contributor Author

kuwv commented Aug 6, 2024

@bitprophet do you have time to look at this patch?

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.

4 participants