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 various type annotations #8046

Merged
merged 21 commits into from
Jun 8, 2024
Merged

Conversation

srittau
Copy link
Contributor

@srittau srittau commented May 7, 2024

Changes proposed in this pull request:

  • This PR add various type annotations, especially for the Image, ImageFont, ImageDraw, and _imagingft modules.

@nulano
Copy link
Contributor

nulano commented May 7, 2024

FWIW I have some WIP type annotations for ImageFont and _imagingft here: main...nulano:Pillow:types-imagefont

@srittau srittau marked this pull request as ready for review May 8, 2024 10:15
@srittau
Copy link
Contributor Author

srittau commented May 8, 2024

I think this is ready for review. The docs build might still fail, but I'd need help with that.

@srittau
Copy link
Contributor Author

srittau commented May 8, 2024

Also, the pypy3.10 failure seems unrelated?

@hugovk
Copy link
Member

hugovk commented May 8, 2024

Yeah, the PyPy one is a bit flaky.

@radarhere
Copy link
Member

The docs build might still fail, but I'd need help with that.

The error

docstring of PIL.Image.Image.transform:1: WARNING: py:class reference target not found: PIL.Image.GetDataTransform

is saying that the GetDataTransform doesn't appear in the documentation, so transform()'s description can't link to it.

The way to solve this is by updating docs/reference/Image.rst to include it, by adding

.. autoclass:: GetDataTransform
    :show-inheritance:

But given that our other protocols so far are SupportsGetMesh and SupportsArrayInterface, maybe this new protocol could be renamed SupportsGetData?

src/PIL/ImageFont.py Outdated Show resolved Hide resolved
src/PIL/_imaging.pyi Outdated Show resolved Hide resolved
src/PIL/Image.py Outdated Show resolved Hide resolved
@@ -34,7 +34,7 @@ def transform(
self,
size: tuple[int, int],
image: Image.Image,
**options: dict[str, str | int | tuple[int, ...] | list[int]],
Copy link
Member

Choose a reason for hiding this comment

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

Could you explain why you made this change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The problem here is that options can be any of the remaining Image.transform arguments, i.e.:

        resample: int = Resampling.NEAREST,
        fill: int = 1,
        fillcolor: float | tuple[float, ...] | str | None 

dict is wrong in any case, but we could annotate it as float | tuple[float, ...] | str | None (with int being redundant with float), but that would imply that any argument could have that type. I see four alternatives:

  • Don't bother and use Any.
  • Use float | tuple[float, ...] | str | None as that would prevent some typing errors, but not all (e.g. fill=(1,2,3) would not be caught), at the cost of a slightly misleading type hints.
  • Just replace options with the actual forwarded arguments. (The option I would probably choose, as there are only three options.)
  • Use Unpack, which means we would need to introduce a new TypedDict for the arguments.

src/PIL/ImageFont.py Outdated Show resolved Hide resolved
srittau and others added 2 commits May 18, 2024 11:22
Co-authored-by: Andrew Murray <3112309+radarhere@users.noreply.github.com>
@radarhere
Copy link
Member

I've created srittau#2 with various suggestions.

@radarhere radarhere merged commit 5bacce9 into python-pillow:main Jun 8, 2024
56 checks passed
Comment on lines -2214 to -2220
size = tuple(size)

self.load()
if box is None:
box = (0, 0) + self.size
else:
box = tuple(box)
Copy link

@lgeiger lgeiger Jul 3, 2024

Choose a reason for hiding this comment

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

@srittau @radarhere This change ended up being a breaking change for me when upgrading from 10.3.0 to 10.4.0 since .resize() previously would accept a numpy array as size. This now fails with an internal error message:

  File "/usr/local/lib/python3.11/site-packages/PIL/Image.py", line 2297, in resize
    if self.size == size and box == (0, 0) + self.size:
       ^^^^^^^^^^^^^^^^^
ValueError: The truth value of an array with more than one element is ambiguous. Use a.any() or a.all()

If this change was intentional I think it would be best to raise a nice error message or revert this and relax the input type requirements.

Copy link
Member

Choose a reason for hiding this comment

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

Let's move this conversation to #8195

Copy link
Member

Choose a reason for hiding this comment

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

#8201 has now restored the ability to use a NumPy array.

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.

5 participants