-
-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
Changes from 5 commits
231d54b
e3932b7
9452536
f613a92
16d4068
d631afc
61d47c3
d49d2d1
530512f
f2228e0
256f3f1
159fc06
a276cf2
a118a82
152a24e
958a651
517b797
4051302
3977124
29dd025
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,5 +1,5 @@ | ||
Internal Reference Docs | ||
======================= | ||
Internal Reference | ||
================== | ||
|
||
.. toctree:: | ||
:maxdepth: 2 | ||
|
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -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): | ||||||||||||||||||||||||
|
@@ -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" | ||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Lines 123 to 133 in 6782a07
I'm not seeing how it would successfully handle There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 if is_path(fp) or isinstance(fp, int): There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. | ||||||||||||||||||||||||
|
||||||||||||||||||||||||
|
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -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) | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For
Suggested change
You can actually also pass There was a problem hiding this comment. Choose a reason for hiding this commentThe 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() | ||||||
|
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 | ||||||||||||||||
|
@@ -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: | ||||||||||||||||
... | ||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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]"] | ||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
Why are the quotes helpful? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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'>
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
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not sure how the
Similar to There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah that's why I suggested adding it similarly to |
||||||||||||||||
|
||||||||||||||||
|
||||||||||||||||
__all__ = ["FileDescriptor", "TypeGuard", "StrOrBytesPath", "SupportsRead"] |
Original file line number | Diff line number | Diff line change | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -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]: | ||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Given that the function returns Lines 232 to 236 in b3a7ae0
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
We haven't added type hints to
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
|
||||||||||||
|
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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).