BlackFoundryCom / robo-cjk

RoboFont extension for CJK font development
GNU General Public License v3.0
15 stars 5 forks source link

prettify .glif MySQL output to GIT #12

Closed BlackFoundry closed 3 years ago

BlackFoundry commented 3 years ago

the XML files should be formated nicely with proper lines and indents

justvanrossum commented 3 years ago

It shouldn't be "prettified": .glif should be formatted exactly as how ufoLib/glifLib saves it, otherwise we can never make changes from the git side without potentally adding format changes. The best way to do that is to build the XML with glifLib.

fabiocaccamo commented 3 years ago

@justvanrossum what do you mean? xml data is the same independently by how it is formatted, I think in this case the prettify is just for readable reasons, @BlackFoundry isn't it?

justvanrossum commented 3 years ago

If I edit a glyph with fonttools based code, and commit it to the repo, I don't want to be distracted by formatting differences. glifLib provides the canonical formatting, why not use it?

BlackFoundry commented 3 years ago

Yes that's the point @fabiocaccamo

justvanrossum commented 3 years ago

The changeover to the MySQL frontend should not have caused any formatting changes whatsoever. (The case renaming of the .rcjk folders was very unfortunate also.)

fabiocaccamo commented 3 years ago

@justvanrossum thank you for the explanation, understood. Actually on server side we don't use glifLib at all, we manage (just read) xml using ElementTree. Using glifLib is it possible just format an xml to obtain the desired result?

justvanrossum commented 3 years ago

Unfortunately it doesn't have this functionality builtin, and this is the shortest I could come up with (only lightly tested):

from fontTools.pens.recordingPen import RecordingPointPen
from fontTools.ufoLib.glifLib import readGlyphFromString, writeGlyphToString

class GlyphObject:
    pass

def reformatGLIF(original):
    glyph = GlyphObject()
    recorder = RecordingPointPen()
    readGlyphFromString(original, glyph, pointPen=recorder)
    return writeGlyphToString(glyph.name, glyph, drawPointsFunc=recorder.replay)

p = 'path/to/glyph.glif'

with open(p) as f:
    original = f.read()
    new = reformatGLIF(original)
    print(new)
fabiocaccamo commented 3 years ago

@justvanrossum I'm sorry for the inconvenience, keeping data formatted identically was not in the requirements, so I thought the only important thing was that the data was not changed. Btw, now that I know it I can fix it easily. Thank you very much for the snippet.

justvanrossum commented 3 years ago

I would have thought a requirement "data was not changed" would mean "in the repo" and therefore implies to include formatting... But I'm glad this is being fixed.

justvanrossum commented 3 years ago

This seems to be done as well. (Again, please reference #N issue numbers in commits and/or PRs.)

fabiocaccamo commented 3 years ago

@justvanrossum yes this has been implemented.

Unfortunately the issue has been opened on the wrong repo, so it was not possible for me to reference the issue in the commit.

To avoid this problem in future the next issues related to backend/api should be opened on the django-robo-cjk repository.

fabiocaccamo commented 3 years ago

@justvanrossum I also compared performance of glifLib vs native xml on 30000 iterations and there is not difference.

justvanrossum commented 3 years ago

Excellent. (But even if there is a difference, given the granularity of the system, I would not expect glifLib to be a bottleneck wrt. responsiveness.)