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

Replace io.BytesIO in type hints #7750

Merged
merged 20 commits into from
Feb 13, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions .coveragerc
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,9 @@ exclude_also =
except ImportError
if TYPE_CHECKING:
@abc.abstractmethod
# Empty bodies in protocols or abstract methods
^\s*def [a-zA-Z0-9_]+\(.*\)(\s*->.*)?:\s*\.\.\.(\s*#.*)?$
^\s*\.\.\.(\s*#.*)?$
Copy link
Member

Choose a reason for hiding this comment

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

My only concern is licensing/permission as this code comes from somewhere else. However, it is in both https://github.com/python/cpython/blob/main/.coveragerc and https://chromium.googlesource.com/external/github.com/python/cpython/+/05e47202a34e6ae05e699af1083455f5b8b59496/.coveragerc, so it may not be unique.

Copy link
Contributor

Choose a reason for hiding this comment

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

IANAL, but my understanding is that you can't copyright code when it is the only (or one of a small number of) obvious solution, which would cover this Regex (I can think of two small changes I might have made if I was making it from scratch, but it is likely possible to enumerate all reasonable solutions on a single page).


[run]
omit =
Expand Down
2 changes: 0 additions & 2 deletions Tests/test_image.py
Original file line number Diff line number Diff line change
Expand Up @@ -162,8 +162,6 @@ def test_stringio(self) -> None:
pass

def test_pathlib(self, tmp_path: Path) -> None:
from PIL.Image import Path

with Image.open(Path("Tests/images/multipage-mmap.tiff")) as im:
assert im.mode == "P"
assert im.size == (10, 10)
Expand Down
23 changes: 5 additions & 18 deletions Tests/test_util.py
Original file line number Diff line number Diff line change
@@ -1,29 +1,16 @@
from __future__ import annotations

from pathlib import Path
from pathlib import Path, PurePath

import pytest

from PIL import _util


def test_is_path() -> None:
# Arrange
fp = "filename.ext"

# Act
it_is = _util.is_path(fp)

# Assert
assert it_is


def test_path_obj_is_path() -> None:
# Arrange
from pathlib import Path

test_path = Path("filename.ext")

@pytest.mark.parametrize(
"test_path", ["filename.ext", Path("filename.ext"), PurePath("filename.ext")]
)
def test_is_path(test_path) -> None:
# Act
it_is = _util.is_path(test_path)

Expand Down
4 changes: 2 additions & 2 deletions docs/reference/internal_design.rst
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
Internal Reference Docs
=======================
Internal Reference
==================

.. toctree::
:maxdepth: 2
Expand Down
8 changes: 8 additions & 0 deletions docs/reference/internal_modules.rst
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,14 @@ Internal Modules
Provides a convenient way to import type hints that are not available
on some Python versions.

.. py:class:: StrOrBytesPath

Typing alias.

.. py:class:: SupportsRead

An object that supports the read method.

.. py:data:: TypeGuard
:value: typing.TypeGuard

Expand Down
2 changes: 1 addition & 1 deletion docs/reference/open_files.rst
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
File Handling in Pillow
=======================

When opening a file as an image, Pillow requires a filename, ``pathlib.Path``
When opening a file as an image, Pillow requires a filename, ``os.PathLike``
object, or a file-like object. Pillow uses the filename or ``Path`` to open a
file, so for the rest of this article, they will all be treated as a file-like
object.
Expand Down
5 changes: 3 additions & 2 deletions src/PIL/GdImageFile.py
Original file line number Diff line number Diff line change
Expand Up @@ -27,11 +27,12 @@ class is not registered for use with :py:func:`PIL.Image.open()`. To open a
"""
from __future__ import annotations

from io import BytesIO
from typing import IO

from . import ImageFile, ImagePalette, UnidentifiedImageError
from ._binary import i16be as i16
from ._binary import i32be as i32
from ._typing import StrOrBytesPath


class GdImageFile(ImageFile.ImageFile):
Expand Down Expand Up @@ -80,7 +81,7 @@ def _open(self) -> None:
]


def open(fp: BytesIO, mode: str = "r") -> GdImageFile:
def open(fp: StrOrBytesPath | IO[bytes], mode: str = "r") -> GdImageFile:
"""
Load texture from a GD image file.

Expand Down
12 changes: 4 additions & 8 deletions src/PIL/Image.py
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,6 @@
import warnings
from collections.abc import Callable, MutableMapping
from enum import IntEnum
from pathlib import Path
from types import ModuleType
from typing import IO, TYPE_CHECKING, Any

Expand Down Expand Up @@ -2383,7 +2382,7 @@ def save(self, fp, format=None, **params) -> None:
implement the ``seek``, ``tell``, and ``write``
methods, and be opened in binary mode.

:param fp: A filename (string), pathlib.Path object or file object.
:param fp: A filename (string), os.PathLike object or file object.
:param format: Optional format override. If omitted, the
format to use is determined from the filename extension.
If a file object was used instead of a filename, this
Expand All @@ -2398,11 +2397,8 @@ def save(self, fp, format=None, **params) -> None:

filename: str | bytes = ""
open_fp = False
if isinstance(fp, Path):
filename = str(fp)
open_fp = True
elif isinstance(fp, (str, bytes)):
filename = fp
if is_path(fp):
filename = os.path.realpath(os.fspath(fp))
open_fp = True
elif fp == sys.stdout:
try:
Expand Down Expand Up @@ -3225,7 +3221,7 @@ def open(fp, mode="r", formats=None) -> Image:
:py:meth:`~PIL.Image.Image.load` method). See
:py:func:`~PIL.Image.new`. See :ref:`file-handling`.

:param fp: A filename (string), pathlib.Path object or a file object.
:param fp: A filename (string), os.PathLike object or a file object.
The file object must implement ``file.read``,
``file.seek``, and ``file.tell`` methods,
and be opened in binary mode. The file object will also seek to zero
Expand Down
7 changes: 3 additions & 4 deletions src/PIL/ImageFont.py
Original file line number Diff line number Diff line change
Expand Up @@ -33,10 +33,10 @@
import warnings
from enum import IntEnum
from io import BytesIO
from pathlib import Path
from typing import BinaryIO

from . import Image
from ._typing import StrOrBytesPath
from ._util import is_directory, is_path


Expand Down Expand Up @@ -193,7 +193,7 @@ class FreeTypeFont:

def __init__(
self,
font: bytes | str | Path | BinaryIO | None = None,
font: StrOrBytesPath | BinaryIO | None = None,
size: float = 10,
index: int = 0,
encoding: str = "",
Expand Down Expand Up @@ -230,8 +230,7 @@ def load_from_bytes(f):
)

if is_path(font):
if isinstance(font, Path):
font = str(font)
font = os.path.realpath(os.fspath(font))
if sys.platform == "win32":
font_bytes_path = font if isinstance(font, bytes) else font.encode()
try:
Expand Down
5 changes: 2 additions & 3 deletions src/PIL/MpegImagePlugin.py
Original file line number Diff line number Diff line change
Expand Up @@ -14,17 +14,16 @@
#
from __future__ import annotations

from io import BytesIO

from . import Image, ImageFile
from ._binary import i8
from ._typing import SupportsRead

#
# Bitstream parser


class BitStream:
def __init__(self, fp: BytesIO) -> None:
def __init__(self, fp: SupportsRead[bytes]) -> None:
self.fp = fp
self.bits = 0
self.bitbuffer = 0
Expand Down
15 changes: 13 additions & 2 deletions src/PIL/_typing.py
Original file line number Diff line number Diff line change
@@ -1,7 +1,8 @@
from __future__ import annotations

import os
import sys
from typing import Sequence, Union
from typing import Protocol, Sequence, TypeVar, Union

if sys.version_info >= (3, 10):
from typing import TypeGuard
Expand All @@ -19,4 +20,14 @@ def __class_getitem__(cls, item: Any) -> type[bool]:
Coords = Union[Sequence[float], Sequence[Sequence[float]]]


__all__ = ["TypeGuard"]
_T_co = TypeVar("_T_co", covariant=True)


class SupportsRead(Protocol[_T_co]):
def read(self, __length: int = ...) -> _T_co: ...


StrOrBytesPath = Union[str, bytes, "os.PathLike[str]", "os.PathLike[bytes]"]
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
StrOrBytesPath = Union[str, bytes, "os.PathLike[str]", "os.PathLike[bytes]"]
StrOrBytesPath = Union[str, bytes, os.PathLike[str], os.PathLike[bytes]]

Why are the quotes helpful?

Copy link
Member Author

Choose a reason for hiding this comment

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

For Python 3.8 compatibility:

❯ python3.8 -m pytest Tests/test_util.py
Traceback (most recent call last):
  File "/Library/Frameworks/Python.framework/Versions/3.8/lib/python3.8/runpy.py", line 194, in _run_module_as_main
    return _run_code(code, main_globals, None,
  File "/Library/Frameworks/Python.framework/Versions/3.8/lib/python3.8/runpy.py", line 87, in _run_code
    exec(code, run_globals)
  File "/Library/Frameworks/Python.framework/Versions/3.8/lib/python3.8/site-packages/pytest/__main__.py", line 5, in <module>
    raise SystemExit(pytest.console_main())
  File "/Library/Frameworks/Python.framework/Versions/3.8/lib/python3.8/site-packages/_pytest/config/__init__.py", line 192, in console_main
    code = main()
  File "/Library/Frameworks/Python.framework/Versions/3.8/lib/python3.8/site-packages/_pytest/config/__init__.py", line 150, in main
    config = _prepareconfig(args, plugins)
  File "/Library/Frameworks/Python.framework/Versions/3.8/lib/python3.8/site-packages/_pytest/config/__init__.py", line 331, in _prepareconfig
    config = pluginmanager.hook.pytest_cmdline_parse(
  File "/Library/Frameworks/Python.framework/Versions/3.8/lib/python3.8/site-packages/pluggy/_hooks.py", line 493, in __call__
    return self._hookexec(self.name, self._hookimpls, kwargs, firstresult)
  File "/Library/Frameworks/Python.framework/Versions/3.8/lib/python3.8/site-packages/pluggy/_manager.py", line 115, in _hookexec
    return self._inner_hookexec(hook_name, methods, kwargs, firstresult)
  File "/Library/Frameworks/Python.framework/Versions/3.8/lib/python3.8/site-packages/pluggy/_callers.py", line 130, in _multicall
    teardown[0].send(outcome)
  File "/Library/Frameworks/Python.framework/Versions/3.8/lib/python3.8/site-packages/_pytest/helpconfig.py", line 104, in pytest_cmdline_parse
    config: Config = outcome.get_result()
  File "/Library/Frameworks/Python.framework/Versions/3.8/lib/python3.8/site-packages/pluggy/_result.py", line 114, in get_result
    raise exc.with_traceback(exc.__traceback__)
  File "/Library/Frameworks/Python.framework/Versions/3.8/lib/python3.8/site-packages/pluggy/_callers.py", line 77, in _multicall
    res = hook_impl.function(*args)
  File "/Library/Frameworks/Python.framework/Versions/3.8/lib/python3.8/site-packages/_pytest/config/__init__.py", line 1075, in pytest_cmdline_parse
    self.parse(args)
  File "/Library/Frameworks/Python.framework/Versions/3.8/lib/python3.8/site-packages/_pytest/config/__init__.py", line 1425, in parse
    self._preparse(args, addopts=addopts)
  File "/Library/Frameworks/Python.framework/Versions/3.8/lib/python3.8/site-packages/_pytest/config/__init__.py", line 1327, in _preparse
    self.hook.pytest_load_initial_conftests(
  File "/Library/Frameworks/Python.framework/Versions/3.8/lib/python3.8/site-packages/pluggy/_hooks.py", line 493, in __call__
    return self._hookexec(self.name, self._hookimpls, kwargs, firstresult)
  File "/Library/Frameworks/Python.framework/Versions/3.8/lib/python3.8/site-packages/pluggy/_manager.py", line 115, in _hookexec
    return self._inner_hookexec(hook_name, methods, kwargs, firstresult)
  File "/Library/Frameworks/Python.framework/Versions/3.8/lib/python3.8/site-packages/pluggy/_callers.py", line 152, in _multicall
    return outcome.get_result()
  File "/Library/Frameworks/Python.framework/Versions/3.8/lib/python3.8/site-packages/pluggy/_result.py", line 114, in get_result
    raise exc.with_traceback(exc.__traceback__)
  File "/Library/Frameworks/Python.framework/Versions/3.8/lib/python3.8/site-packages/pluggy/_callers.py", line 77, in _multicall
    res = hook_impl.function(*args)
  File "/Library/Frameworks/Python.framework/Versions/3.8/lib/python3.8/site-packages/_pytest/config/__init__.py", line 1153, in pytest_load_initial_conftests
    self.pluginmanager._set_initial_conftests(
  File "/Library/Frameworks/Python.framework/Versions/3.8/lib/python3.8/site-packages/_pytest/config/__init__.py", line 563, in _set_initial_conftests
    self._try_load_conftest(anchor, importmode, rootpath)
  File "/Library/Frameworks/Python.framework/Versions/3.8/lib/python3.8/site-packages/_pytest/config/__init__.py", line 580, in _try_load_conftest
    self._getconftestmodules(anchor, importmode, rootpath)
  File "/Library/Frameworks/Python.framework/Versions/3.8/lib/python3.8/site-packages/_pytest/config/__init__.py", line 609, in _getconftestmodules
    mod = self._importconftest(conftestpath, importmode, rootpath)
  File "/Library/Frameworks/Python.framework/Versions/3.8/lib/python3.8/site-packages/_pytest/config/__init__.py", line 657, in _importconftest
    self.consider_conftest(mod)
  File "/Library/Frameworks/Python.framework/Versions/3.8/lib/python3.8/site-packages/_pytest/config/__init__.py", line 739, in consider_conftest
    self.register(conftestmodule, name=conftestmodule.__file__)
  File "/Library/Frameworks/Python.framework/Versions/3.8/lib/python3.8/site-packages/_pytest/config/__init__.py", line 498, in register
    self.consider_module(plugin)
  File "/Library/Frameworks/Python.framework/Versions/3.8/lib/python3.8/site-packages/_pytest/config/__init__.py", line 747, in consider_module
    self._import_plugin_specs(getattr(mod, "pytest_plugins", []))
  File "/Library/Frameworks/Python.framework/Versions/3.8/lib/python3.8/site-packages/_pytest/config/__init__.py", line 754, in _import_plugin_specs
    self.import_plugin(import_spec)
  File "/Library/Frameworks/Python.framework/Versions/3.8/lib/python3.8/site-packages/_pytest/config/__init__.py", line 781, in import_plugin
    __import__(importspec)
  File "<frozen importlib._bootstrap>", line 991, in _find_and_load
  File "<frozen importlib._bootstrap>", line 975, in _find_and_load_unlocked
  File "<frozen importlib._bootstrap>", line 671, in _load_unlocked
  File "/Library/Frameworks/Python.framework/Versions/3.8/lib/python3.8/site-packages/_pytest/assertion/rewrite.py", line 178, in exec_module
    exec(co, module.__dict__)
  File "/Users/hugo/github/Pillow/Tests/helper.py", line 19, in <module>
    from PIL import Image, ImageMath, features
  File "/Users/hugo/github/Pillow/src/PIL/Image.py", line 61, in <module>
    from ._util import DeferredError, is_path
  File "/Users/hugo/github/Pillow/src/PIL/_util.py", line 7, in <module>
    from ._typing import StrOrBytesPath, TypeGuard
  File "/Users/hugo/github/Pillow/src/PIL/_typing.py", line 29, in <module>
    StrOrBytesPath = Union[str, bytes, os.PathLike[str], os.PathLike[bytes]]
TypeError: 'ABCMeta' object is not subscriptable
❯ python3.8
Python 3.8.10 (v3.8.10:3d8993a744, May  3 2021, 09:09:08)
[Clang 12.0.5 (clang-1205.0.22.9)] on darwin
Type "help", "copyright", "credits" or "license" for more information.
>>> import os
>>> os.PathLike[str]
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: 'ABCMeta' object is not subscriptable
❯ python3.9
Python 3.9.13 (v3.9.13:6de2ca5339, May 17 2022, 11:37:23)
[Clang 13.0.0 (clang-1300.0.29.30)] on darwin
Type "help", "copyright", "credits" or "license" for more information.
>>> import os
>>> os.PathLike[str]
os.PathLike[str]
>>> type(os.PathLike[str])
<class 'types.GenericAlias'>

types.GenericAlias is new in Python 3.9:

https://docs.python.org/3/whatsnew/3.9.html#collections-abc

So we can drop the quotes in October when 3.8 goes EOL.

Shall we do this, so pyupgrade catches it when we drop 3.8?

Suggested change
StrOrBytesPath = Union[str, bytes, "os.PathLike[str]", "os.PathLike[bytes]"]
if sys.version_info >= (3, 9):
StrOrBytesPath = Union[str, bytes, os.PathLike[str], os.PathLike[bytes]]
else:
StrOrBytesPath = Union[str, bytes, "os.PathLike[str]", "os.PathLike[bytes]"]

Copy link

@Viicos Viicos Feb 6, 2024

Choose a reason for hiding this comment

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

Not sure how the typing-extensions dependency is managed on Pillow, but TypeAlias could be used as well:

StrOrBytesPath: TypeAlias = "str | bytes | os.PathLike[str] | os.PathLike[bytes]"

Similar to TypeGuard, a "dummy" implementation of TypeAlias could be added in _typing.py?

Copy link
Member

Choose a reason for hiding this comment

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

Maybe you're already aware but TypeAlias was only added in Python 3.10 - https://docs.python.org/3/library/typing.html#typing.TypeAlias

I'm personally fine with the current version. We can just remove the quotes by hand in October when Python 3.8 is EOL.

Copy link

Choose a reason for hiding this comment

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

Yeah that's why I suggested adding it similarly to TypeGuard. But yes probably not worth for just a few months



__all__ = ["TypeGuard", "StrOrBytesPath", "SupportsRead"]
9 changes: 4 additions & 5 deletions src/PIL/_util.py
Original file line number Diff line number Diff line change
@@ -1,17 +1,16 @@
from __future__ import annotations

import os
from pathlib import Path
from typing import Any, NoReturn

from ._typing import TypeGuard
from ._typing import StrOrBytesPath, TypeGuard


def is_path(f: Any) -> TypeGuard[bytes | str | Path]:
return isinstance(f, (bytes, str, Path))
def is_path(f: Any) -> TypeGuard[StrOrBytesPath]:
Copy link
Contributor

Choose a reason for hiding this comment

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

Given that the function returns isinstance(f, (bytes, str, Path)), I'm not sure I understand the point of this change.
I haven't checked, but I think this will reintroduce a type warning in ImageFont here:

Pillow/src/PIL/ImageFont.py

Lines 232 to 236 in b3a7ae0

if is_path(font):
if isinstance(font, Path):
font = str(font)
if sys.platform == "win32":
font_bytes_path = font if isinstance(font, bytes) else font.encode()

Copy link

@AlexWaygood AlexWaygood Jan 25, 2024

Choose a reason for hiding this comment

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

Good point — I think I gave @hugovk bad (or at least incomplete) advice here.

I would probably consider refactoring the code you link to there in ImageFont.py so that it does handle any os.PathLike rather than specifically checking for pathlib.Path, and change this function so that it does isinstance(f, (str, bytes, os.PathLike)) instead of isinstance(f, (str, bytes, pathlib.Path)). But perhaps all that could be deferred to another PR

Copy link
Member Author

Choose a reason for hiding this comment

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

I haven't checked, but I think this will reintroduce a type warning in ImageFont here:

We haven't added type hints to ImageFont.py yet, it's giving 55 strict warnings for both main and this PR, and zero non-strict on both.

I would probably consider refactoring the code you link to there in ImageFont.py so that it does handle any os.PathLike rather than specifically checking for pathlib.Path, and change this function so that it does isinstance(f, (str, bytes, os.PathLike)) instead of isinstance(f, (str, bytes, pathlib.Path)). But perhaps all that could be deferred to another PR

I've updated this PR to include the refactor.

return isinstance(f, (bytes, str, os.PathLike))


def is_directory(f: Any) -> TypeGuard[bytes | str | Path]:
def is_directory(f: Any) -> TypeGuard[StrOrBytesPath]:
"""Checks if an object is a string, and that it points to a directory."""
return is_path(f) and os.path.isdir(f)

Expand Down
Loading