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

Added type hints for PixelAccess related methods and others #8032

Merged
merged 21 commits into from
Jun 25, 2024

Conversation

nulano
Copy link
Contributor

@nulano nulano commented Apr 29, 2024

No description provided.

@@ -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]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This cast is required on Windows and macOS.

src/PIL/PyAccess.py Outdated Show resolved Hide resolved
@@ -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`
Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

Be aware that this will become simpler after Pillow 10.4.0, as PyAccess will end deprecation and be removed.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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:
Copy link
Member

Choose a reason for hiding this comment

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

This could be ImageFile.ImageFile, rather than Image.Image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

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 Show resolved Hide resolved
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]
Copy link
Member

Choose a reason for hiding this comment

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

The test suite does provide a list for xy

def test_list(self) -> None:
im = hopper()
assert im.getpixel([0, 0]) == (20, 20, 70)

Also, why use SupportsInt, and not just int?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also, why use SupportsInt, and not just int?

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

Pillow/src/_imaging.c

Lines 1165 to 1170 in 114e017

if (PyLong_Check(value)) {
*x = PyLong_AS_LONG(value);
} else if (PyFloat_Check(value)) {
*x = (int)PyFloat_AS_DOUBLE(value);
} else {
PyObject *int_value = PyObject_CallMethod(value, "__int__", NULL);

However, I see now that this doesn't work for self.pyaccess which requires an actual int and rejects a float.

@radarhere radarhere mentioned this pull request Jun 3, 2024
@radarhere
Copy link
Member

I've merged some of these changes in #8099

radarhere and others added 3 commits June 25, 2024 10:50
Co-authored-by: Ondrej Baranovič <ondreko.tiba@gmail.com>
@radarhere radarhere merged commit d2b5e11 into python-pillow:main Jun 25, 2024
56 checks passed
@nulano nulano deleted the type_hints branch June 25, 2024 09:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants