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 type hints to _util #7642

Merged
merged 6 commits into from
Jan 1, 2024
Merged

Add type hints to _util #7642

merged 6 commits into from
Jan 1, 2024

Conversation

nulano
Copy link
Contributor

@nulano nulano commented Dec 27, 2023

Helps #2625.

I wanted to start adding type hints for PIL.ImageFont, but it is still in the list of excluded files.

The errors in ImageFont can be fixed by adding type hints for PIL._util and an empty _imagingft.pyi file.
There are three issues in ImageFont:

  • error: Module "PIL" has no attribute "_imagingft"

    Solved by adding empty _imagingft.pyi file, which reveals another error:

    error: Incompatible types in assignment (expression has type "DeferredError", variable has type Module)

    Solved by adding _util.DeferredError.new(ex) -> Any.

  • error: Missing type parameters for generic type "IO"

    I think this should be typing.BinaryIO, Handle pathlib.Path in FreeTypeFont #7578 (comment).
    Note that typeshed has _typeshed.SupportsRead[bytes].

  • several more errors due to failed propagation of the _util.is_path type guard

    Solved by adding type hints for _util.is_path. This requires the use of typing.TypeGuard which is Python 3.10+ only. For Python<=3.9, it is taken from typing_extensions. See https://mypy.readthedocs.io/en/stable/runtime_troubles.html#using-new-additions-to-the-typing-module for more information.

I've also included fixes for errors in ImageFile (incorrect ImageFile._Tile.args annotation) and ImageCms (missing _imagingcms.pyi file), as they are quite simple.

@nulano
Copy link
Contributor Author

nulano commented Dec 27, 2023

Thinking about this some more, the typing_extensions dependency can be made optional with something like:

if sys.version_info >= (3, 10):
    from typing import TypeGuard

    PathGuard = TypeGuard[bytes | str | Path]
elif typing.TYPE_CHECKING:
    from typing_extensions import TypeGuard

    PathGuard = TypeGuard[bytes | str | Path]
else:
    PathGuard = bool


def is_path(f) -> PathGuard:
    return isinstance(f, (bytes, str, Path))

However, I'm not sure if this is worth doing to avoid the hard dependency, and if so, how the dependency should be declared in pyproject.toml.

@hugovk
Copy link
Member

hugovk commented Dec 27, 2023

Other than for testing, we've managed to avoid Python dependencies so far.

I doubt adding typing_extensions would be a big problem, but if we want to keep zero dependencies, we could move this if/elif/else block into a _typing.py or _compat.py module.

Another option is to leave typing_extensions out, like:

if sys.version_info >= (3, 10):
    from typing import TypeGuard

    PathGuard = TypeGuard[bytes | str | Path]
else:
    PathGuard = bool

And use new Python for type checking; we use 3.12 on the CI. This could also be tidied into an internal module.

@nulano
Copy link
Contributor Author

nulano commented Dec 27, 2023

After a lot of trial and error, I've managed to make typing_extensions an optional dependency.
As it turns out, my initial suggestion was not functional because mypy doesn't understand a TypeGuard when it is provided as a TypeAlias. The only way I could convince it to accept the type guard in ImageFont was to (in)directly import TypeGuard from either typing or typing_extensions, and provide a fallback class when neither is available (which mypy will never see).

Even if we later add a hard dependency on typing_extensions, I do like the idea of moving the if/else block to _typing.py.

Co-authored-by: Hugo van Kemenade <hugovk@users.noreply.github.com>
@radarhere radarhere merged commit 57096f5 into python-pillow:main Jan 1, 2024
56 of 57 checks passed
@nulano nulano deleted the types-util branch January 1, 2024 10:43
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