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

(Even) better merger errors #2226

Merged
merged 20 commits into from
Mar 19, 2021
Merged

Conversation

simoncozens
Copy link
Collaborator

Fixes #2224 (as well as some other silent errors) and adds a test.

Lib/fontTools/varLib/errors.py Show resolved Hide resolved
Lib/fontTools/varLib/merger.py Outdated Show resolved Hide resolved
Lib/fontTools/varLib/errors.py Outdated Show resolved Hide resolved
Tests/varLib/varLib_test.py Outdated Show resolved Hide resolved
@simoncozens
Copy link
Collaborator Author

Thanks, good catches!

@anthrotype anthrotype requested a review from madig March 17, 2021 12:10
@madig
Copy link
Contributor

madig commented Mar 17, 2021

I realize it's probably too late for a debate on principles, but is there a special reason why there's an enum and error classes instead of subclassing the base VarLibMergeError for each subcategory of VarLibMergeFailure? Would make instantiation and printing a bit more straightforward I suppose...

@simoncozens
Copy link
Collaborator Author

Maybe. I’ll take a look at doing it that way.

@simoncozens
Copy link
Collaborator Author

Done, but now the code is sufficiently different, you're going to have to review it again. :-)

@madig
Copy link
Contributor

madig commented Mar 19, 2021

Hm, this scuppers CFF merge errors, they follow their own logic. Maybe the merge errors with a cause should be subclasses of a class VarLibDetailedMergeError(VarLibMergeError) for now. Or someone needs to adapt the CFF code... Also, could the __init__s use kwargs so one can do

UnsupportedFormat(self, subtable="pair positioning lookup")
# UnsupportedFormat(self, ({"subtable": "pair positioning lookup"},))

InconsistentGlyphOrder(self)
# InconsistentGlyphOrder(self, ({},))

@anthrotype
Copy link
Member

@madig good catch about CFF-related VarLibMergeErrors (I'd expected the tests to fail?)

also 👍 on using **kwargs

@madig
Copy link
Contributor

madig commented Mar 19, 2021

The tests don't fail, but the str makes little sense.

@simoncozens
Copy link
Collaborator Author

Yes, moving to kwargs was a good move; it allowed me to split up the reason from the traceback, which makes everything a lot tidier.

@madig
Copy link
Contributor

madig commented Mar 19, 2021

Small nit: if your class has a docstring, you don't need a pass.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Output message for GPOS error
3 participants