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

Add minimal typing (and assorted cleanups) #266

Closed
wants to merge 19 commits into from

Conversation

stephenfin
Copy link

This PR started out as an alternative to #225 that integrated type hints inline. However, it has since grown and now accomplishes the following:

  • Drop Python 3.6, 3.7 support;
  • Migrate to tox v4;
  • Remove unused compat helpers for Python < 3.8;
  • Integrate the pre-commit tool and ruff linter, code formatter;
  • and, last but not least, add the mypy linter and minimal type hints

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.

These are both EOL [1]. Users can continue using an older version of
wrapt where needed.

[1] https://devguide.python.org/versions/

Signed-off-by: Stephen Finucane <stephen@that.guru>
tox 4 doesn't seem to like the use of setup.cfg. We can also remove the
use of a custom install command for Python 3.11 since coverage has fixed
the binary packages for same.

Signed-off-by: Stephen Finucane <stephen@that.guru>
We can just use the built-in variant as-is now.

Signed-off-by: Stephen Finucane <stephen@that.guru>
Just use 'str'.

Signed-off-by: Stephen Finucane <stephen@that.guru>
Signed-off-by: Stephen Finucane <stephen@that.guru>
Signed-off-by: Stephen Finucane <stephen@that.guru>
Signed-off-by: Stephen Finucane <stephen@that.guru>
Signed-off-by: Stephen Finucane <stephen@that.guru>
This is not necessary in Python 3-only code.

Signed-off-by: Stephen Finucane <stephen@that.guru>
Another py2/py3 diff.

Signed-off-by: Stephen Finucane <stephen@that.guru>
Signed-off-by: Stephen Finucane <stephen@that.guru>
@GrahamDumpleton
Copy link
Owner

That is a lot to take in. Let me take it in a bit at a time and comment as necessary.

@GrahamDumpleton
Copy link
Owner

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.

For running linters. Only a minimal configuration is enabled for now.

Signed-off-by: Stephen Finucane <stephen@that.guru>
Signed-off-by: Stephen Finucane <stephen@that.guru>
Signed-off-by: Stephen Finucane <stephen@that.guru>
Signed-off-by: Stephen Finucane <stephen@that.guru>
Signed-off-by: Stephen Finucane <stephen@that.guru>
@stephenfin
Copy link
Author

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.

@GrahamDumpleton
Copy link
Owner

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

@stephenfin
Copy link
Author

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.

@GrahamDumpleton
Copy link
Owner

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

This is *super* early, but it add hints for the two most commonly used
entry points for wrapt.

Signed-off-by: Stephen Finucane <stephen@that.guru>
@stephenfin
Copy link
Author

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.

Without suffix we end up using pypy2, which is incompatible.

Signed-off-by: Stephen Finucane <stephen@that.guru>
@GrahamDumpleton
Copy link
Owner

@stephenfin
Copy link
Author

stephenfin commented Jul 5, 2024

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 😅

Signed-off-by: Stephen Finucane <stephen@that.guru>
@Hugovdberg
Copy link

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]

@GrahamDumpleton
Copy link
Owner

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.

@stephenfin
Copy link
Author

stephenfin commented Jul 25, 2024

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

@Hugovdberg
Copy link

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.

@dimaqq
Copy link

dimaqq commented Sep 26, 2024

I've managed to scribble together this: https://gist.github.com/dimaqq/e4b418e1b9ce6a3cd874ba9540fe42c6

@Kludex
Copy link

Kludex commented Dec 10, 2024

This is far from being minimal. It's far easier to make this happen if first we drop the PY2, then the other Python versions (3.6, and 3.7), and then add the type hints... And we don't need to add all type hints at once.

I'm happy to implement that order first, and being more didactic on the type hints, so @GrahamDumpleton can learn more about the decisions, rather than just accepting this huge PR.

Please give a thumbs up, and I'll do it.

@GrahamDumpleton
Copy link
Owner

I had been trying to get onto this when had some extended time off working recently but too many other things were happening. I am now on a trip so can’t do any hands on stuff myself until January but I can perhaps review stuff if done in small bits and merge. Let’s summarize where things are at.

  1. Python 2.7 support has now been dropped from wrapt.
  2. Suppport for < Python 3.8 has been dropped.
  3. There is still a C extension.
  4. I suspect changes made in wrapt since this PR was done means it may not apply too cleanly and so may need to start over with new PR and we do it a step at a time, being reviewed at each step and merged before proceeding so easier if we want to progress with this while on my trip, since I can only review code/tests and merge and little else.

As preparation for making changes this time it would make sense to remove anything related to Python 2.7 as first step. This manifests in some of the tests (inside test files and whole test files which are specific to Python version) and in both the C extension code and pure Python code from memory.

Next issue is that although we could now embed type hints in the pure Python code given we are Python 3.9+, how do we stand with doing than when in most cases the C extension is what will be imported. I can’t remember if the fact that we have a C extension, and pure Python code is optionally used, whether editors can still work it out. This PR seems to have made changes in the Python code rather than separate type hints file, so if someone can remind me how does that work for C extensions?

Now there was a separate PR for dropping support for Python <3.8 which did more changes than was actually done but it also is likely out of date. It though could still be used as a guide. So as very first step if someone could draft a PR for just dropping Python <3.8 from tests. We can review that and merge. Then we can start looking at what needs to be removed from actual code for old Python versions.

@Kludex
Copy link

Kludex commented Dec 11, 2024

Dropping 2.7: #279

@stephenfin stephenfin closed this Dec 11, 2024
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.

5 participants