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

Improve ImageFont error messages #8338

Merged
merged 18 commits into from
Sep 18, 2024
Merged
Show file tree
Hide file tree
Changes from 15 commits
Commits
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
29 changes: 28 additions & 1 deletion Tests/test_imagefont.py
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
import re
import shutil
import sys
import tempfile
from io import BytesIO
from pathlib import Path
from typing import Any, BinaryIO
Expand Down Expand Up @@ -460,17 +461,43 @@ def test_free_type_font_get_mask(font: ImageFont.FreeTypeFont) -> None:
assert mask.size == (108, 13)


def test_load_when_image_not_found() -> None:
with tempfile.NamedTemporaryFile(delete=False) as tmp:
pass
with pytest.raises(OSError) as e:
ImageFont.load(tmp.name)

os.unlink(tmp.name)

root = os.path.splitext(tmp.name)[0]
assert str(e.value) == f"cannot find glyph data file {root}.{{gif|pbm|png}}"


def test_load_path_not_found() -> None:
# Arrange
filename = "somefilenamethatdoesntexist.ttf"

# Act/Assert
with pytest.raises(OSError):
with pytest.raises(OSError) as e:
ImageFont.load_path(filename)

# The file doesn't exist, so don't suggest `load`
assert filename in str(e.value)
assert "did you mean" not in str(e.value)
with pytest.raises(OSError):
ImageFont.truetype(filename)


def test_load_path_existing_path() -> None:
with tempfile.NamedTemporaryFile() as tmp:
with pytest.raises(OSError) as e:
ImageFont.load_path(tmp.name)

# The file exists, so the error message suggests to use `load` instead
assert tmp.name in str(e.value)
assert " did you mean" in str(e.value)


def test_load_non_font_bytes() -> None:
with open("Tests/images/hopper.jpg", "rb") as f:
with pytest.raises(OSError):
Expand Down
22 changes: 15 additions & 7 deletions src/PIL/ImageFont.py
Original file line number Diff line number Diff line change
Expand Up @@ -98,11 +98,13 @@ class ImageFont:
def _load_pilfont(self, filename: str) -> None:
with open(filename, "rb") as fp:
image: ImageFile.ImageFile | None = None
root = os.path.splitext(filename)[0]

for ext in (".png", ".gif", ".pbm"):
if image:
image.close()
try:
fullname = os.path.splitext(filename)[0] + ext
fullname = root + ext
image = Image.open(fullname)
except Exception:
pass
Expand All @@ -112,7 +114,8 @@ def _load_pilfont(self, filename: str) -> None:
else:
if image:
image.close()
msg = "cannot find glyph data file"

msg = f"cannot find glyph data file {root}.{{gif|pbm|png}}"
raise OSError(msg)

self.file = fullname
Expand Down Expand Up @@ -224,7 +227,7 @@ def __init__(
raise core.ex

if size <= 0:
msg = "font size must be greater than 0"
msg = f"font size must be greater than 0, not {size}"
Copy link
Member

Choose a reason for hiding this comment

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

Was this also a source of confusion for you? I'm not sure why this wouldn't be suitably obvious.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No this happened more on auto-pilot while coding in the evening -- I pretty much always try to include the incorrect value in error messages if it fails on input-validation as I find it makes debugging quite a lot easier for me (especially if the error is several frames down the stack from where I'm working).

Feel free to remove it, no hard feelings there :)

raise ValueError(msg)

self.path = font
Expand Down Expand Up @@ -768,8 +771,9 @@ def getlength(self, text: str | bytes, *args: Any, **kwargs: Any) -> float:

def load(filename: str) -> ImageFont:
"""
Load a font file. This function loads a font object from the given
bitmap font file, and returns the corresponding font object.
Load a font file. This function loads a font object from the given
bitmap font file, and returns the corresponding font object. For loading TrueType
or OpenType fonts instead, see :py:func:`~PIL.ImageFont.truetype`.

:param filename: Name of font file.
:return: A font object.
Expand All @@ -789,7 +793,8 @@ def truetype(
) -> FreeTypeFont:
"""
Load a TrueType or OpenType font from a file or file-like object,
and create a font object.
and create a font object. For loading bitmap fonts instead,
see :py:func:`~PIL.ImageFont.load` and :py:func:`~PIL.ImageFont.load_path`.
This function loads a font object from the given file or file-like
object, and creates a font object for a font of the given size.
radarhere marked this conversation as resolved.
Show resolved Hide resolved

Expand Down Expand Up @@ -927,7 +932,10 @@ def load_path(filename: str | bytes) -> ImageFont:
return load(os.path.join(directory, filename))
except OSError:
pass
msg = "cannot find font file"
msg = f"cannot find font file '{filename}' in sys.path"
yngvem marked this conversation as resolved.
Show resolved Hide resolved
if os.path.exists(filename):
msg += f', did you mean ImageFont.load("{filename}") instead?'

raise OSError(msg)


Expand Down
Loading