Added type hints for PixelAccess related methods and others #8032
Conversation
src/PIL/ImageGrab.py
Outdated
| @@ -63,6 +69,7 @@ def grab(bbox=None, include_layered_windows=False, all_screens=False, xdisplay=N | |||
| left, top, right, bottom = bbox | |||
| im = im.crop((left - x0, top - y0, right - x0, bottom - y0)) | |||
| return im | |||
| xdisplay = cast(Union[str, None], xdisplay) # type: ignore[redundant-cast, unused-ignore] | |||
There was a problem hiding this comment.
This cast is required on Windows and macOS.
| @@ -847,7 +881,7 @@ def load(self): | |||
| operations. See :ref:`file-handling` for more information. | |||
|
|
|||
| :returns: An image access object. | |||
| :rtype: :ref:`PixelAccess` or :py:class:`PIL.PyAccess` | |||
| :rtype: :py:class:`.PixelAccess` or :py:class:`.PyAccess` | |||
There was a problem hiding this comment.
My initial reaction to this was to feel confused , because the return type is PixelAccess or None, but this says PixelAccess or PyAccess.
Of course, the return type uses PixelAccess the protocol, not the class. But should the protocol have a different name perhaps to avoid confusion? SupportsPixelAccess?
There was a problem hiding this comment.
Be aware that this will become simpler after Pillow 10.4.0, as PyAccess will end deprecation and be removed.
There was a problem hiding this comment.
I think that after 10.4.0, we'll have no need for the protocol - we can just use core.PixelAccess. If so, then it's not helpful to add a public protocol only to remove it in the next version. I've created nulano#39 to return core.PixelAccess | PyAccess.PyAccess | None instead.
There was a problem hiding this comment.
Fair enough, I've pushed a similar commit, see comment in nulano#39.
| @@ -94,7 +101,7 @@ def grab(bbox=None, include_layered_windows=False, all_screens=False, xdisplay=N | |||
| return im | |||
|
|
|||
|
|
|||
| def grabclipboard(): | |||
| def grabclipboard() -> Image.Image | list[str] | None: | |||
There was a problem hiding this comment.
This could be ImageFile.ImageFile, rather than Image.Image
There was a problem hiding this comment.
I'm not opposed to changing this to ImageFile, but would that unnecessarily lock us in to a given return type?
Or can we change return types without a major version bump and deprecation period?
There was a problem hiding this comment.
I hope that we can change return types without deprecation, but I can appreciate that we could be a bit forwards-compatible here. There's also no reason for the user to expect ImageFile instead of an Image. So I think either form is fine.
src/PIL/Image.py
Outdated
| @@ -1627,7 +1660,9 @@ def apply_transparency(self): | |||
|
|
|||
| del self.info["transparency"] | |||
|
|
|||
| def getpixel(self, xy): | |||
| def getpixel( | |||
| self, xy: tuple[SupportsInt, SupportsInt] | |||
There was a problem hiding this comment.
The test suite does provide a list for xy
Pillow/Tests/test_image_access.py
Lines 217 to 219 in 95a69ec
Also, why use SupportsInt, and not just int?
There was a problem hiding this comment.
Also, why use
SupportsInt, and not justint?
I wanted to reflect that this is passed to a C function that calls __int__ on the provided coordinates (unless they are an int or float):
Lines 1165 to 1170 in 114e017
However, I see now that this doesn't work for
self.pyaccess which requires an actual int and rejects a float.
|
I've merged some of these changes in #8099 |
for more information, see https://pre-commit.ci
Co-authored-by: Ondrej Baranovič <ondreko.tiba@gmail.com>
No description provided.