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 5 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 @@ -10,6 +10,9 @@ exclude_also =
if DEBUG:
# Don't complain about compatibility code for missing optional dependencies
except ImportError
# 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
23 changes: 6 additions & 17 deletions Tests/test_util.py
Original file line number Diff line number Diff line change
@@ -1,27 +1,16 @@
from __future__ import annotations

from pathlib import Path, PurePath

import pytest

from PIL import _util


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

# Act
it_is = _util.is_path(fp)

# Assert
assert it_is


def test_path_obj_is_path():
# 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):
# 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
12 changes: 12 additions & 0 deletions docs/reference/internal_modules.rst
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,18 @@ Internal Modules
Provides a convenient way to import type hints that are not available
on some Python versions.

.. py:class:: FileDescriptor

Typing alias.

.. py:class:: StrOrBytesPath
hugovk marked this conversation as resolved.
Show resolved Hide resolved

Typing alias.

.. py:class:: SupportsRead

An object that supports the read method.

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

Expand Down
7 changes: 5 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 FileDescriptor, StrOrBytesPath


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


def open(fp: BytesIO, mode: str = "r") -> GdImageFile:
def open(
fp: StrOrBytesPath | FileDescriptor | IO[bytes], mode: str = "r"
Copy link
Member

Choose a reason for hiding this comment

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

fp is used to create GdImageFile through its parent ImageFile, and is then handled by

Pillow/src/PIL/ImageFile.py

Lines 123 to 133 in 6782a07

if is_path(fp):
# filename
self.fp = open(fp, "rb")
self.filename = fp
self._exclusive_fp = True
else:
# stream
self.fp = fp
self.filename = filename
# can be overridden
self._exclusive_fp = None

I'm not seeing how it would successfully handle FileDescriptor?

Choose a reason for hiding this comment

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

Good point, if you wanted to also handle file descriptors, you'd need to change line 123 of ImageFile.py to:

    if is_path(fp) or isinstance(fp, int):

Copy link
Member

Choose a reason for hiding this comment

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

GdImageFile is not exactly the most used image plugin, so I don't think we need to be adding new functionality for it.

I've created hugovk#113 to remove the FileDescriptor.

Choose a reason for hiding this comment

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

Yeah, this was my mistake — I think I misread something. Agree that supporting file descriptors isn't really necessary

) -> GdImageFile:
"""
Load texture from a GD image file.

Expand Down
2 changes: 1 addition & 1 deletion src/PIL/ImageFont.py
Original file line number Diff line number Diff line change
Expand Up @@ -230,7 +230,7 @@ def load_from_bytes(f):
)

if is_path(font):
if isinstance(font, Path):
if isinstance(font, os.PathLike):
font = str(font)

Choose a reason for hiding this comment

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

For pathlib.Paths, str(path_object) always produces what you want here, but that's not necessarily true for all os.PathLikes. You should use os.fspath here if you want the code to work with arbitrary PathLikes:

Suggested change
font = str(font)
font = os.fspath(font)

You can actually also pass str and bytes objects to os.fspath, so you could probably get rid of the isinstance(font, os.PathLike) check immediately above as well (I'm on my mobile so my suggestion can't span multiple lines :( )

Choose a reason for hiding this comment

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

if sys.platform == "win32":
font_bytes_path = font if isinstance(font, bytes) else font.encode()
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
16 changes: 15 additions & 1 deletion src/PIL/_typing.py
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
from __future__ import annotations

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

if sys.version_info >= (3, 10):
from typing import TypeGuard
Expand All @@ -15,4 +17,16 @@ def __class_getitem__(cls, item: Any) -> type[bool]:
return bool


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


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

Choose a reason for hiding this comment

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

Looks like coverage is complaining about this line "not being covered by tests" — here's how we deal with this in CPython, FWIW: https://github.com/python/cpython/blob/d22c066b802592932f9eb18434782299e80ca42e/.coveragerc#L12

(I added the exclude when we started adding type hints to Argument Clinic ;)



FileDescriptor = int
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__ = ["FileDescriptor", "TypeGuard", "StrOrBytesPath", "SupportsRead"]
6 changes: 3 additions & 3 deletions src/PIL/_util.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,11 +4,11 @@
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]:
hugovk marked this conversation as resolved.
Show resolved Hide resolved
Expand Down
Loading