-
Notifications
You must be signed in to change notification settings - Fork 51
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
update GlyphData from Glyphs 3.2 #959
Conversation
A few tests need to be updated and most of the regression tests failures look like improvements, except may be this one? ! tests/data/gf/DotGothic16/DotGothic16-Regular.ufo/lib.plist - /tmp/tmpzlyzjnmk/DotGothic16-Regular.ufo/lib.plist---
+++
@@ -11190,9 +11190,9 @@
<key>micro.rotat</key>
<string>uni00B5.rotat</string>
<key>middledot-kata.half</key>
- <string>uniFF65</string>
+ <string>middledotkata.half</string>
<key>middledot-kata.half.rotat</key>
- <string>uniFF65.rotat</string>
+ <string>middledotkata.half.rotat</string>
<key>miriSquare</key>
<string>uni3349</string>
<key>miriSquare.vert</key> |
There was an altName in that is not in my data. |
I don’t think glyphsLib uses this. It only cares about GDEF, so Mark/[Nonspacing,Spacing Combining], and */Ligature: glyphsLib/Lib/glyphsLib/builder/features.py Lines 202 to 219 in 3554be4
|
It does seem to look at the altName. That way it can connect the glyph name "middledot-kata.half" to the info entry "dot-kata.half" and get its production name. It could have used the fallback to search with the unicode, but that doesn't seem to work. |
Can anyone summarize where this stands, and if anything is holding this PR up? |
Was this copied from the files in https://github.com/schriftgestalt/GlyphsInfo? Usually we write the commit hash in the commit message when we update these files to know where they came from. Can you do that? |
@schriftgestalt ping... |
11b7854
to
a3e9788
Compare
a3e9788
to
daa96da
Compare
The regression tests need to be updated. There are a lot fails with the production name of the So it needs to run |
The regression tests will fix themselves automatically after you merge the PR |
@schriftgestalt are you able to take any action to move this forward? E.g., along the lines of @anthrotype's comment #959 (comment) ? |
I can’t do anything else on this. As I understand cosimos comment is that the test will work after it is merged? |
i think so, let's see |
"Ll": ("Letter", "Lowercase"), | ||
"Ll": ("Letter", None), | ||
"Lm": ("Letter", "Modifier"), | ||
"Lo": ("Letter", None), | ||
"Lt": ("Letter", "Uppercase"), | ||
"Lu": ("Letter", "Uppercase"), | ||
"Lt": ("Letter", None), | ||
"Lu": ("Letter", None), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@schriftgestalt why this change? What does it have to do with updating the GlyphData database?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see you commented on this already
The case information moved from subcategory="Upper/Lowercase" to case="upper/lower". Not sure if that is used anywhere.
If it were me, i'd just have left it as it was. Let's hope nothing else breaks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But it was failing some tests.
the regression tests passed |
Putting that up to see what the test are doing ;).