10000 Fix decorator in retry module by fperrin · Pull Request #5482 · python/typeshed · GitHub
[go: up one dir, main page]
Skip to content

Fix decorator in retry module #5482

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 3 commits into from
May 17, 2021
Merged

Fix decorator in retry module #5482

merged 3 commits into from
May 17, 2021

Conversation

fperrin
Copy link
Contributor
@fperrin fperrin commented May 17, 2021

Hi,

I'm afraid the type annotations for the retry module are incorrect, although it's possible that the real issue is in mypy; it's possible I don't understand typing annotations enough to be 100% sure.

I want to typecheck the following program with mypy:

from retry import retry

@retry(tries=5)
def foo(bar: str) -> int: ...

# Valid code
bar: int
bar = foo("abc")

# Should fail (twice): both argument and return value have incompatible types
baz: dict
baz = foo([])

reveal_type(retry)

Running with python3.7 (as provided by Debian) or 3.9.4 (latest version compiled locally) with mypy 0.820 and types-retry 0.1.1, I get the following:

% mypy demo.py
demo.py:3: error: Argument 1 has incompatible type "Callable[[str], int]"; expected <nothing>
demo.py:8: error: <nothing> not callable
demo.py:12: error: <nothing> not callable
demo.py:14: note: Revealed type is "def [_T <: def (*Any, **Any) -> Any] (exceptions: Union[Type[builtins.Exception], builtins.tuple[Type[builtins.Exception]]] =, tries: builtins.int =, delay: builtins.float =, max_delay: Union[builtins.float, None] =, backoff: builtins.float =, jitter: Union[Tuple[builtins.float, builtins.float], builtins.float] =, logger: Union[logging.Logger, None] =) -> def (_T`-1) -> _T`-1"
Found 3 errors in 1 file (checked 1 source file)

Which is not right.

With the changes included in this PR, I get the expected errors from mypy:

% env MYPYPATH=$PWD/stubs/retry mypy demo.py
demo.py:12: error: Incompatible types in assignment (expression has type "int", variable has type "Dict[Any, Any]")
demo.py:12: error: Argument 1 to "foo" has incompatible type "List[<nothing>]"; expected "str"
demo.py:14: note: Revealed type is "def (exceptions: Union[Type[builtins.Exception], builtins.tuple[Type[builtins.Exception]]] =, tries: builtins.int =, delay: builtins.float =, max_delay: Union[builtins.float, None] =, backoff: builtins.float =, jitter: Union[Tuple[builtins.float, builtins.float], builtins.float] =, logger: Union[logging.Logger, None] =) -> retry.api._Decorator"
Found 2 errors in 1 file (checked 1 source file)

I copied the definition of _Decorator from stubs/click/click/decorators.pyi.

hauntsaninja
hauntsaninja approved these changes May 17, 2021
View reviewed changes
Copy link
Collaborator
@hauntsaninja hauntsaninja left a comment

Choose a reason for hiding this comment

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

Thanks, this looks good to me.

For the record, I think this might be a bug in mypy, since:

diff --git a/stubs/retry/retry/api.pyi b/stubs/retry/retry/api.pyi
index 88f274b9..6e1a0edd 100644
--- a/stubs/retry/retry/api.pyi
+++ b/stubs/retry/retry/api.pyi
@@ -25,4 +25,4 @@ def retry(
     backoff: float = ...,
     jitter: Union[Tuple[float, float], float] = ...,
     logger: Optional[Logger] = ...,
-) -> _Decorator[_T]: ...
+) -> Callable[[_T], _T]: ...

is also enough to fix.

However, IIRC this shorthand where the TypeVar only occurs in the return type is sort of contentious syntax between type checkers, so using the callback protocol is probably for the best.

@hauntsaninja
Copy link
Collaborator

Dug up the thread: https://mail.python.org/archives/list/typing-sig@python.org/thread/DDR2B3USYCQGVXM3ZSK2FASH5YFQGPUV/

@srittau
Copy link
Collaborator
srittau commented May 17, 2021

I extracted IdentityFunction to _typeshed in #5483, like I said we should. We can use that in this PR.

@fperrin
Copy link
Contributor Author
fperrin commented May 17, 2021

Thanks for having a look @hauntsaninja .
@srittau , should I wait for #5483 to be merged and update this PR? Or you want to make the change to retry yourself?

@srittau
Copy link
Collaborator
srittau commented May 17, 2021

Whatever you prefer, I can do it, or you can wait.

@fperrin
Copy link
Contributor Author
fperrin commented May 17, 2021

Finish your work on 5483, I'll take care of retry later.

@srittau
Copy link
Collaborator
srittau commented May 17, 2021

#5483 is merged now.

@fperrin
Copy link
Contributor Author
fperrin commented May 17, 2021

Merged master and updated to use IdentifyFunction.

@srittau srittau merged commit a0abacd into python:master May 17, 2021
@fperrin
Copy link
Contributor Author
fperrin commented May 17, 2021

Thanks @srittau & @hauntsaninja !

@hauntsaninja hauntsaninja mentioned this pull request Jun 11, 2021
Closed
@JHeilCoveo JHeilCoveo mentioned this pull request Jun 25, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JelleZijlstra JelleZijlstra JelleZijlstra approved these changes

@hauntsaninja hauntsaninja hauntsaninja approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@fperrin @hauntsaninja @srittau @JelleZijlstra
0