Identify the culprit glif in read errors with a loose backport of PEP678

This commit annotates errors from GlyphSet.readGlyph() with the details
of the glyph that originated them (e.g. name, path to glif). This is
implemented with a loose backport of PEP678, to avoid adding a wrapper
error that would be less specific and would break API compatibility.

In addition, this commit adds a test to ensure that the new details are
present (specifically, in the case of parsing invalid XML).
This commit is contained in:
Harry Dalton 2023-03-14 15:58:18 +00:00
parent 1f3380300b
commit 70ca6dec9b
3 changed files with 46 additions and 15 deletions

View File

@ -1,3 +1,6 @@
from __future__ import annotations
class UFOLibError(Exception):
pass
@ -7,7 +10,12 @@ class UnsupportedUFOFormat(UFOLibError):
class GlifLibError(UFOLibError):
pass
def _add_note(self, note: str) -> None:
# Loose backport of PEP 678 until we only support Python 3.11+, used for
# adding additional context to errors.
# TODO: Replace with https://docs.python.org/3.11/library/exceptions.html#BaseException.add_note
(message, *rest) = self.args
self.args = ((message + "\n" + note), *rest)
class UnsupportedGLIFFormat(GlifLibError):

View File

@ -416,17 +416,33 @@ class GlyphSet(_UFOBaseIO):
if validate is None:
validate = self._validateRead
text = self.getGLIF(glyphName)
tree = _glifTreeFromString(text)
formatVersions = GLIFFormatVersion.supported_versions(
self.ufoFormatVersionTuple
)
_readGlyphFromTree(
tree,
glyphObject,
pointPen,
formatVersions=formatVersions,
validate=validate,
)
try:
tree = _glifTreeFromString(text)
formatVersions = GLIFFormatVersion.supported_versions(
self.ufoFormatVersionTuple
)
_readGlyphFromTree(
tree,
glyphObject,
pointPen,
formatVersions=formatVersions,
validate=validate,
)
except GlifLibError as glifLibError:
# Re-raise with a note that gives extra context, describing where
# the error occurred.
fileName = self.contents[glyphName]
try:
glifLocation = f"'{self.fs.getsyspath(fileName)}'"
except fs.errors.NoSysPath:
# Network or in-memory FS may not map to a local path, so use
# the best string representation we have.
glifLocation = f"'{fileName}' from '{str(self.fs)}'"
glifLibError._add_note(
f"The issue is in glyph '{glyphName}', located in {glifLocation}."
)
raise
def writeGlyph(
self,

View File

@ -138,7 +138,8 @@ class GlyphSetTests(unittest.TestCase):
def testReadGlyphInvalidXml(self):
"""Test that calling readGlyph() to read a .glif with invalid XML raises
a library error, instead of an exception from the XML dependency that is
used internally."""
used internally. In addition, check that the raised exception describes
the glyph by name and gives the location of the broken .glif file."""
# Create a glyph set with three empty glyphs.
glyph_set = GlyphSet(self.dstDir)
@ -150,10 +151,16 @@ class GlyphSetTests(unittest.TestCase):
invalid_xml = b"<abc></def>"
Path(self.dstDir, glyph_set.contents["c"]).write_bytes(invalid_xml)
# Confirm that reading /c raises an appropriate error.
# Confirm that reading /a and /b is fine...
glyph_set.readGlyph("a", _Glyph())
glyph_set.readGlyph("b", _Glyph())
with pytest.raises(GlifLibError, match="GLIF contains invalid XML"):
# ...but that reading /c raises a descriptive library error.
expected_message = (
r"GLIF contains invalid XML\.\n"
r"The issue is in glyph 'c', located in '.*c\.glif.*\."
)
with pytest.raises(GlifLibError, match=expected_message):
glyph_set.readGlyph("c", _Glyph())