GrahamDumpleton / wrapt

A Python module for decorators, wrappers and monkey patching.
BSD 2-Clause "Simplified" License
2.03k stars 231 forks source link

make wrapt a PEP561 typed package. #225

Open bitranox opened 1 year ago

bitranox commented 1 year ago

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

bitranox commented 1 year ago

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

GrahamDumpleton commented 1 year ago

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

bitranox commented 1 year ago

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

dimaqq commented 1 year ago

Kind poke!

GrahamDumpleton commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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

:muscle: :bear:

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

...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! :love_hotel:

bitranox commented 1 year ago

@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

GrahamDumpleton commented 5 months ago

Latest version of wrapt requires Python 3.6+ and in https://github.com/GrahamDumpleton/wrapt/pull/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.