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

How to handle input UFO with bad glyph names? #271

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

belluzj
Copy link
Collaborator

@belluzj belluzj commented Aug 27, 2018

When someone tries to compile with fontmake a UFO that has an invalid glyph name (in this case it was "G," with a literal comma in the name), the bad glyph name goes verbatim into the output feature code and fontmake complains about the syntax being wrong. When trying to understand the problem, it's a bit confusing because the original feature only has valid syntax, and the syntax error lies in what ufo2ft writes out.

I have added tests that cover this bug. What do you think we should do?

  1. Not care: the bad naming was obviously a mistake, and it doesn't happen often enough to warrant a proper error message
  2. Make ufo2ft raise when glyph names are wrong: in my opinion ufo2ft should always output compilable feature code, or raise if that's not possible.
  3. @moyogo suggests that this should not be an issue in the first place, and ufo2ft/fontmake should rename glyphs to production names on the fly and don't care about crazy glyph names

@madig madig requested a review from anthrotype August 27, 2018 13:45
@anthrotype
Copy link
Member

personally, I'm leaning towards option 1 (not care, bad glyph name as user's mistake).
Option 3 is not straightforward, because the production glyph renaming occurs after we have compiled the features.
If somebody wants to implement option 2 (raise meaningful error on invalid glyph names) you're welcome

Base automatically changed from master to main March 1, 2021 09:01
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.

2 participants