Open stephenfin opened 2 months ago
That is a lot to take in. Let me take it in a bit at a time and comment as necessary.
If have time please check results of GitHub actions build https://github.com/GrahamDumpleton/wrapt/actions/runs/9742980097 to see if you can see why changes failing. Thanks.
If have time please check results of GitHub actions build GrahamDumpleton/wrapt/actions/runs/9742980097 to see if you can see why changes failing. Thanks.
Looks like I missed a comma in the pypy-3.10
factor of [gh-actions] python
setting in tox.ini
. Fixed now.
Next issue failing with Python 3.9.
File "/Users/runner/work/wrapt/wrapt/src/wrapt/__init__.py", line 25, in <module>
from .decorators import (
File "/Users/runner/work/wrapt/wrapt/src/wrapt/decorators.py", line 172, in <module>
) -> F | partial[F]:
TypeError: unsupported operand type(s) for |: 'TypeVar' and 'types.GenericAlias'
I don't know if is just Python 3.9 since once it starts failing jobs in GitHub actions it bails out on running others.
Example: https://github.com/GrahamDumpleton/wrapt/actions/runs/9765079127/job/26972836883?pr=266
Next issue failing with Python 3.9.
File "/Users/runner/work/wrapt/wrapt/src/wrapt/__init__.py", line 25, in <module> from .decorators import ( File "/Users/runner/work/wrapt/wrapt/src/wrapt/decorators.py", line 172, in <module> ) -> F | partial[F]: TypeError: unsupported operand type(s) for |: 'TypeVar' and 'types.GenericAlias'
I don't know if is just Python 3.9 since once it starts failing jobs in GitHub actions it bails out on running others.
Example: GrahamDumpleton/wrapt/actions/runs/9765079127/job/26972836883?pr=266
Ah, indeed. I need to use ty.Union
there (or defer evaluation by using strings instead). Let me spin again.
Now Python 3.8 is failing.
File "/home/runner/work/wrapt/wrapt/src/wrapt/__init__.py", line 25, in <module>
from .decorators import (
File "/home/runner/work/wrapt/wrapt/src/wrapt/decorators.py", line 155, in <module>
) -> partial[F]: ...
TypeError: 'type' object is not subscriptable (key ~F)
https://github.com/GrahamDumpleton/wrapt/actions/runs/9780347915/job/27015921033?pr=266
Now Python 3.8 is failing.
File "/home/runner/work/wrapt/wrapt/src/wrapt/__init__.py", line 25, in <module> from .decorators import ( File "/home/runner/work/wrapt/wrapt/src/wrapt/decorators.py", line 155, in <module> ) -> partial[F]: ... TypeError: 'type' object is not subscriptable (key ~F)
https://github.com/GrahamDumpleton/wrapt/actions/runs/9780347915/job/27015921033?pr=266
Okay, I actually ran all the tests locally this time. Sorry for the noise. Hopefully this gets to the bottom of things.
As an aside, we might want to explore dropping Python 3.8 support. EOL is in October, a mere 3 months from now, and typing is significantly improved in 3.9.
Getting closer.
Doesn't look like that's due to anything here. Rather, it looks like we need to bump the version of the pypa/cibuildwheel
action. I've pushed a follow-up. The rest of the actions looks like they need an update but this PR is already far larger than it has any right to be :sweat_smile:
would it be ok to add the typing_extensions
package as install dependency? It is quite lightweight, and commonly installed, and backports modern typing features to older versions of Python. If we could include that the typing hints could make use of typing.ParamSpec, which allows much more information to be preserved on the wrapped function. For python 3.10+ (or 3.8+ with typing extensions) we can do this:
OldReturnType = TypeVar("OldReturnType")
ReturnType = TypeVar("ReturnType")
ParamType = ParamSpec("ParamType")
AdapterType = TypeVar("AdapterType", bound=Callable[..., Any])
class _Wrapper(Protocol[ParamType, OldReturnType, ReturnType]):
"""Specification of what a wrapper function should look like"""
def __call__(
self,
wrapped: Callable[ParamType, OldReturnType],
instance: Optional[Any],
args: tuple[Any, ...],
kwargs: dict[str, Any],
) -> ReturnType: ...
# Specification of the decorator returned by @wrapt.decorator
_Decorator = Callable[[Callable[ParamType, OldReturnType]], Callable[ParamType, ReturnType]]
...
@overload
def decorator(
wrapper: None = None,
enabled: Optional[bool] = None,
adapter: Optional[AdapterType] = None,
proxy: Type[FunctionWrapper] = FunctionWrapper,
) -> Callable[[_Wrapper[ParamType, OldReturnType, ReturnType]], _Decorator[ParamType, OldReturnType, ReturnType]]: ...
@overload
def decorator(
wrapper: _Wrapper[ParamType, OldReturnType, ReturnType],
enabled: Optional[bool] = None,
adapter: Optional[AdapterType] = None,
proxy: Type[FunctionWrapper] = FunctionWrapper,
) -> _Decorator[ParamType, OldReturnType, ReturnType]: ...
def decorator(
wrapper: Optional[_Wrapper[ParamType, OldReturnType, ReturnType]] = None,
enabled: Optional[bool] = None,
adapter: Optional[AdapterType] = None,
proxy: Type[FunctionWrapper] = FunctionWrapper,
) -> Union[
_Decorator[ParamType, OldReturnType, ReturnType],
Callable[[_Wrapper[ParamType, OldReturnType, ReturnType]], _Decorator[ParamType, OldReturnType, ReturnType]],
]:
This way type checkers not only know the return type of the decorator is a function, but specifically a function that takes the same inputs as the original function but returns the return type of the wrapper:
@wrapt.decorator
def catch_all(wrapped: Callable[ParamType, ReturnType], instance, args, kwargs) -> Optional[ReturnType]:
try:
return wrapped(*args, **kwargs)
except:
return None
@catch_all
def inverse(x: int) -> float:
return 1/x
# inverse: (x: int) -> Optional[float]
My only concern with having the dependency is whether we might encounter issues if users of wrapt themselves are using typing_extensions for other reasons, or is used by other packages, and they want an incompatible version. Do you see any potential issues like that?
Only asked since they talk in the docs about how versioning was applied in typing_extensions has changed over time. How safe is it going to be require typing_extensions version >= 4.0.
BTW, sorry for not getting on top of the PR. Have had a lot of other work and life stuff to deal with this year.
@Hugovdberg Feel free to refine the typing as much as you'd like but perhaps we can get this in as-is for now and do that refinement in follow-up PRs? As things stand, this is already large enough that it might warrant being split into multiple PRs (though I'm hoping my thorough commit messages are enough to avoid that). I don't really want to add even more to it.
EDIT: Alternatively, I can pull the typing change out of this series, retitle it to "assorted cleanups" or similar, and let you propose a new PR based on this that adds typing. Happy with either. Those "assorted cleanups" were mainly intended to make adding typing easier so they should benefit you.
I think you're right in proposing to merge this as it is, if @GrahamDumpleton could merge this PR I will create a new PR to implement my suggestions for further improved type hints.
This PR started out as an alternative to #225 that integrated type hints inline. However, it has since grown and now accomplishes the following:
I would advise looking at the individual commits rather than the PR as a whole, since it's likely to be much more approachable. Any questions, please ask.
This replaces #225, #261 and #191.