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

make wrapt a PEP561 typed package. #225

Open
wants to merge 3 commits into
base: develop
Choose a base branch
from

Conversation

bitranox
Copy link

not sure if the name have to be "wrapt.pyi" or "py.typed" - that needs to be tested.

not sure if the name have to be "wrapt.pyi" or "py.typed" - that needs to be tested.
@bitranox
Copy link
Author

correction : not sure if the name have to be "wrapt.pyi" or "wrappers.pyi" - that needs to be tested.

@GrahamDumpleton
Copy link
Owner

Is the .idea directory an editor directory and should it be removed and then name added to .gitignore?

@bitranox
Copy link
Author

oh, sorry, yes, the .idea needs to be excluded

@dimaqq
Copy link

dimaqq commented May 30, 2023

Kind poke!

@GrahamDumpleton
Copy link
Owner

The problem is that I don't know enough about adding python type hinting to a package to know whether this patch even works. From my testing it is incomplete for a number of reasons.

The first problem is that the py.typed and *.pyi files don't get installed with the package. This is solved by adding to setup.cfg the lines:

[options.package_data]
wrapt = py.typed, *.pyi

The second problem is that the type hints were included in a file called wrapt.pyi, but that is ignored. The pyi files should have a basename which matches the particular code file they contain hints for.

Thus, should at least perhaps be named __init__.pyi instead and the type hint for wrapt.decorator is then at least recognised by pylance under VS Code when type checking is set to basic. I would have thought they should have been in decorators.pyi and wrappers.pyi respectively, although I couldn't get that working, but that may actually be because the package can optionally use a C extension, so maybe it should be _wrappers.pyi for some things. I am not really sure whether this is an issue with the type hints or how I have the editor set up, since I have never used type checking for Python in the editor before.

Anyway, for the type hint for wrapt.decorator, addressing the above issue doesn't actually seem to help though as the editor still complains about the decorated function.

image

There is also another problem which is that as soon as you add a .pyi file, it is taken as the authoritative source for interface information, thus the contents on the py file are ignored. This means all the other functions and types exported by the wrapt module are not recognised at all.

image

So it seems you can't do this half way and as soon as you start using pyi files then they need to be complete and contain interface descriptions for everything in the module they sit parallel to. So having type hints for just wrapt.decorator and wrapt.ObjectProxy is not enough and they are needed for everything else in wrapt as well, which is going to be much more work.

What I would perhaps suggest for anyone who better understands this and wants to see it done, is to create a separate Python package in a GitHub repository (not released to PyPi), which is a -stubs package per PEP 561 and try and construct a complete type hints package for wrapt which captures everything required. For those who want to test it while is worked on, it can be installed from the GitHub repository.

The only other option I can see is to wait until Python 2 support is dropped which would allow type hints to be included in the Python code itself, although separate pyi files are likely still going to be needed to define prototypes for anything implemented in the C extension. How that will work though I don't know when wrapt can depending on how it is installed use a pure Python variant of code, or the C extension variant.

@dimaqq
Copy link

dimaqq commented Jun 12, 2023

I think you're correct.
If wrapt supports both py2 and py3, then separate pyi files are a must.

Typically, there's 1-1 for .py and .pyi file (technically one for each module), like in
https://github.com/python/typeshed/tree/main/stubs/redis/redis

At the same time, I think (but I can't find the reference) that stub package can be shipped in a form of a single file, and that would be wrapt/wrapt.pyi.

@bitranox
Copy link
Author

bitranox commented Jun 12, 2023

  • python2 sunset date has passed January 1, 2020, so I guess its time to clean up and get rid of the python2 code.
    The current available wrapt version will be still available on pypy for python2 users, but You can enjoy the python3 possibilities.

  • I use the type hints to be included in the Python code itself in my code, and that is because my modules are not that matured, so it is a pain to keep *.py and *.pyi files in sync, but the readability of the code suffers a bit when put that type information in the *py files.

For a very matured project like wrapt I think its better to work with *.pyi files, because signatures dont change that frequently.

There is also another problem which is that as soon as you add a .pyi file, it is taken as the authoritative source for interface information, thus the contents on the py file are ignored. This means all the other functions and types exported by the wrapt module are not recognised at all.

I never worked with pylance , I use pycharm - I am almost sure with pycharm that is not the case, but i did not try.

So it seems you can't do this half way and as soon as you start using pyi files then they need to be complete and contain interface descriptions for everything in the module they sit parallel to.

I would guess that 99% of the users only use the wrapt decorator and nothing else. So, to type check that, the signature of the wrapt decorator would be enough, and You can add signatures gradually as needed.
as @dimaqq pointed out, stub files can be marked as partial, so most people will be happy with type hints for the wrap decorator only :)

Besides the "offline" type checkers, there is also a wonderful runtime type checker :

https://github.com/beartype/beartype

maybe You can team up with @leycec - he is a "Runtime type-checking aficionado. "

@dimaqq
Copy link

dimaqq commented Jun 12, 2023

I would guess that 99% of the users only use the wrapt decorator and nothing else.

stubs can be marked as partial, this makes sense.
I'd be happy with type hints for the wrap decorator only :)

@leycec
Copy link

leycec commented Jun 12, 2023

maybe You can team up with @leycec - he is a "Runtime type-checking aficionado. "

💪 🐻

@leycec has been summoned to the chat. The chat proceeds to descend into chaos.

Actually, I humbly agree with everything @bitranox has wisely spouted above – except this:

I use the type hints to be included in the Python code itself in my code, and that is because my modules are not that matured, so it is a pain to keep *.py and *.pyi files in sync...

Yes. I strongly agree with this. In 2023, I see vanishingly few .pyi files in open-source projects. Indeed, I'll strengthen this claim; the only open-source projects with .pyi files in 2023 are projects with C extensions (e.g., NumPy), which cannot be annotated with type hints in pure C and thus require external .pyi files for annotations.

Pure-Python .py files, however, are always annotated with type hints directly in those files themselves. Why? Because, as @bitranox observed:

  • Desynchronization. It's all too easy for .py and .pyi files to diverge and all too hard to detect that divergence, which leads directly to...
  • Testing. It's unclear whether pytest-based unit or integration tests even can programmatically detect a desynchronization between .py and .pyi files. You'll never even know when you've done wrong.
  • Runtime type-checking. .pyi files are mostly incompatible with runtime type-checkers like @beartype, Pydantic, and typeguard; .py files are perfectly compatible. Indeed, @beartype is on the cusp of releasing import hooks enabling end users to type-check their entire app stack (including third-party dependencies like wrapt) at runtime in only two lines of code. But this only applies to type hints defined in .py files; type hints defined in .pyi files are basically opaque and inaccessible to runtime type-checkers. This is why my emoji sobs unconsolably. 😭

...but the readability of the code suffers a bit when put that type information in the *py files.

Actually, it's probably the opposite; well-annotated .py files tend to be more readable than unannotated .py files if anything, as the type hints annotating those files explicitly inform readers of the types accepted and returned by classes and callables in those files.

That said, your mileage may vary (YMMV). This has been a public service announcement from the @beartype Broadcasting Service. Please do let me know if there is anything @beartype can explicitly do for wrapt here. So much love for all the awesome good work you do for the Python community, wrapt devs! 🏩

@bitranox
Copy link
Author

@leycec , always funny and precise comments, love it.
@GrahamDumpleton , the minimum You can do is to introduce a partial pyi file, and decorate at least the wrapt decorator - 99% of users (including me) will be happy with that.
As soon as You get rid of the python2 compatibility, You can start to annotate in the *.py files, enable us coding slaves to enjoy runtime type checkers like beartype

Comment on lines +5 to +9
T = TypeVar("T", bound=Any)

def decorator(wrapper: F, enabled: Optional[bool] = None, adapter: Optional[A] = None) -> F: ...

class ObjectProxy(Generic[T]):
Copy link

Choose a reason for hiding this comment

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

I think that pyi files should also follow pep-8, which means 2 blank lines between functions and classes declared at the module scope.

Maybe just run ruff or black to auto-format this file?

A = TypeVar('A', bound=Callable[..., Any])
T = TypeVar("T", bound=Any)

def decorator(wrapper: F, enabled: Optional[bool] = None, adapter: Optional[A] = None) -> F: ...
Copy link

Choose a reason for hiding this comment

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

Due to microsoft/pyright#5929 the re-export in __init__.py needs to be updated too.

For example, from .decorators import decorator as decorator

The issue affects pyright but not mypy.

You can test this by e.g.

poetry add git+https://github.com/GrahamDumpleton/wrapt.git#a2dddf6e10536f4cd4bfc96ff1a706cd71b0694c
poetry add pyright
poetry add mypy

pyright some_test_file.py
mypy some_test_file.py

Copy link

Choose a reason for hiding this comment

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

An alternative to from ... import A as A would be to provide explicit __all__ in wrapt/__init__.py.

@GrahamDumpleton
Copy link
Owner

Latest version of wrapt requires Python 3.6+ and in #261 someone recently suggested requiring Python 3.8+.

So maybe someone wants to have a go at a separate PR now which embeds annotations in the code. Would appreciate it starting with core functionality first so I can follow along, learn, and verify myself since am not knowledgeable about using type annotations.

Copy link

@dimaqq dimaqq left a comment

Choose a reason for hiding this comment

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

This did not work for me, so I managed to type the wrapt.decorator in a different way, I think it's Py 3.7+

from typing import Any, Callable, Optional, TypeVar, Tuple, Dict, Protocol

R = TypeVar('R', covariant=True)  # Return type of the function
T = TypeVar('T')  # Type of instance (for methods)

class FuncProtocol(Protocol[R]):
    def __call__(self, *args: Any, **kwargs: Any) -> R: ...

def decorator(
    wrapper: Callable[
        [
            FuncProtocol[R],
            Optional[T],
            Tuple[Any, ...],
            Dict[str, Any]],
        R],
    enabled: Optional[bool] = None
) -> FuncProtocol[R]: ...

https://gist.github.com/dimaqq/e4b418e1b9ce6a3cd874ba9540fe42c6

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