unified-font-object / ufoNormalizer

A tool that will normalize the XML and other data inside of a UFO.
Other
50 stars 19 forks source link

whitespace and empty tags #53

Open schriftgestalt opened 5 years ago

schriftgestalt commented 5 years ago

How did you decide how the intendation and empty elements are handled.

I compared the .plist files generated by the Normalizer and what macOS (Xcode) would produce and found significantly differences. RoboFont agrees in this points with macOS.

1) The top level dict element is indented in the normalised file and not in the file written by Xcode. from the Normalizer:

<?xml version="1.0" encoding="UTF-8"?>
<!DOCTYPE plist PUBLIC "-//Apple//DTD PLIST 1.0//EN" "http://www.apple.com/DTDs/PropertyList-1.0.dtd">
<plist version="1.0">
    <dict>
        <key>ascender</key>
        <integer>1491</integer>

from Xcode

<?xml version="1.0" encoding="UTF-8"?>
<!DOCTYPE plist PUBLIC "-//Apple//DTD PLIST 1.0//EN" "http://www.apple.com/DTDs/PropertyList-1.0.dtd">
<plist version="1.0">
<dict>
    <key>ascender</key>
    <integer>1491</integer>

2) empty elements: from the Normalizer:

        <key>openTypeOS2Selection</key>
        <array>
        </array>

from Xcode:

    <key>openTypeOS2Selection</key>
    <array/>

I tested this in Xcode on 10.9 and 10.13.

benkiel commented 5 years ago

I believe that the whitespace preferences came from the older version of plistlib that the ufoLib used to use. However, this is the reason for the normalizer, so that the writing libs can change, but you still get meaningful diffs (see the optional use of lxml writer in UFOlib now, etc). It serves as a benchmark, not the only way to write out XML, but one that can be relied on over time to produce the same result, no matter the tool, so the whitespace and empty element preferences are set as they are.

schriftgestalt commented 5 years ago

so the whitespace and empty element preferences are set as they are

But how did you pick that particular preference? Wouldn’t it make sense to follow the dtd?

Why deviate from several reference implementations?

benkiel commented 5 years ago

@schriftgestalt To be very clear, I did not write the code for this. My best recollection/guess is that the preference was set by how ufoLib wrote out UFOs in 2016, which is a reference standard and implementation.

I don't think that there is much value is arguing about whitespace and empty elements: many folks use the normalizer in production workflows, so changing the preference now would mean a lot of noise in version control, the very thing that the normalizer is made to avoid.

anthrotype commented 5 years ago

the formatting is based upon what ElementTree does (as that is the library that the normalizer uses to write XML)

actually, ufoNormalizer uses its own XMLWriter class, ElementTree is only used for parsing.

https://github.com/unified-font-object/ufoNormalizer/blob/ea37f454f14f77334196595afcf0201a6ac19417/src/ufonormalizer.py#L1151

benkiel commented 5 years ago

@anthrotype my bad, memory failed me and I didn't double check closely enough.

miguelsousa commented 5 years ago

Regardless, you provided an excellent description of the main goal of the normalizer:

I don't think that there is much value is arguing about whitespace and empty elements: many folks use the normalizer in production workflows, so changing the preference now would mean a lot of noise in version control, the very thing that the normalizer is made to avoid.

I'm inclined to close this issue.

Alhadis commented 4 years ago

so changing the preference now would mean a lot of noise in version control

That's probably the same reason <array>\n</array> is used instead of <array/>. Compare:

    <array>
+       <string>New entry</string>
    </array>

… as opposed to:

-   <array/>
+   <array>
+       <string>New entry</string>
+   </array>
schriftgestalt commented 4 years ago

Maybe adding a second profile that collects all the cleanup an synchronizations. So people can choose when or if they switch.