[glifLib] Wrap XML library exceptions with glifLib types when parsing glifs #3029
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This allows dependent projects to catch errors parsing glifs without requiring logic to account for which XML library fonttools is using internally (e.g. for implementing fonttools/ufoLib2#264). This commit also adds tests to ensure that the exception we expose when glifs have invalid syntax remains stable across future releases.
anthrotype
reviewed
Mar 9, 2023
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).
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Hello!
This PR abstracts across XML library exceptions with glifLib errors when parsing glyphs, which allows dependent projects to catch syntax errors without requiring logic to account for which XML library fonttools is using internally (e.g. for implementing fonttools/ufoLib2#264).
The PR also also adds tests to ensure that the exception we expose remains stable across future releases.
Affected functions
glifLib.readGlyphFromString()glifLib.GlyphSet.readGlyph()Alternative
If we wanted to abstract over lxml/xml exceptions more broadly, we could catch them in
etree.pyand wrap them with a fonttools-specific XmlParsingError; this would also have the cost of maintaining wrapper functions (e.g. foretree.fromstring()) where there currently are none.