source-foundry / ufolint

UFO source format linter
24 stars 5 forks source link

Idea: explicitly look for merge conflict markers #23

Open madig opened 6 years ago

madig commented 6 years ago

Happens every so often. They are caught anyway because they break the XML, but the error messages may be cryptic.

Maybe adapt https://github.com/pre-commit/pre-commit-hooks/blob/master/pre_commit_hooks/check_merge_conflict.py to ufolint.

anthrotype commented 6 years ago

why not just use that?

madig commented 6 years ago

Possible, but introduces pre-commit. Then you need to go and set up caches for it because the setup takes a minute or more...

chrissimpkins commented 6 years ago

Read every UFO text file and test for these markers? Or are they happening at a significant frequency in specific file area so that we can narrow the list?

anthrotype commented 6 years ago

pre-commit is primarily for local development. you install it once then it does its thing. anyway, I don't use ufolint, you do what you need to do

anthrotype commented 6 years ago

more interesting if you turned ufolint into a pre-commit hook (ok now I shut up :grin:)

chrissimpkins commented 6 years ago

no that's an interesting idea Cosimo. Details?

anthrotype commented 6 years ago

https://pre-commit.com/ https://github.com/pre-commit/pre-commit https://github.com/pre-commit/pre-commit-hooks

chrissimpkins commented 6 years ago

@madig what are you seeing in the error message when this happens?

anthrotype commented 6 years ago

a pre-commit hook can be any script that exits with nonzero on failure; it can also modify files in the working directory (e.g. see black)

chrissimpkins commented 6 years ago

Problem is getting contributors to set this up... :)

anthrotype commented 6 years ago

i'm sure Nikolaus is already concocting something :wink:

madig commented 6 years ago

Problem is getting contributors to set this up... :)

This is a sticking point. I'm using a build tool wrapper internally at work but not everyone uses it, and people struggle with Git as is. I set up ufolint on the CI for two projects today, but the designers were very concerned about false positives preventing them from building the fonts on the CI (font developers are under constant time pressure), so I allow the job to fail and proceed to building anyway. Setting this up as a pre-commit hook would make little sense like that. We'll see.

what are you seeing in the error message when this happens?

test.ufo.zip

[...]
[FAIL] a test failed with error: not well-formed (invalid token): line 6, column 1
[...]
chrissimpkins commented 6 years ago
  1. ufolint does not have false positives! Damn Brits. No wonder we forked America. :)
  2. why can't they build locally with their changes in order to proof the work that they've done? Is this where the merge conflicts are occurring as they attempt to pull down work from other designers in different branches so that they can see everyone's changes in a given day?
  3. UFO merges are headache inducing. I am looking forward to seeing your UFO merge tool... At the moment I don't let anyone commit anything other than .glif files in a collaborative workflow. They either add the rest of the data in the UFO source to .gitignore or they .developerignore it by not hitting commit on anything other than a .glif. This places the burden of glyph additions, removals, etc on the maintainer which is no fun but otherwise the source becomes a mess as the editors rearrange glyph name lists, add library metadata, etc. And we are not dealing with kerning, OT features, and the like. I can't even imagine.
madig commented 6 years ago
  1. They hesitate with changes to the workflow due to the aforementioned pressure ;)
  2. Some build locally, some don't. We're talking about designers here, some of which are surprisingly CLI-savvy, the rest of which revolt in horror at the mere thought. They frequently merge stuff back and forth and every so often, someone (inadvertently) touches something they shouldn't have (hello color marks)... or rename the UFO and someone was on holidays and continues working on his old branch and tries to merge...
    1. Yes.
    2. Use ufonormalizer -m some.ufo to enforce order.
    3. One tricky area outside .glif files is metadata in e.g. fontinfo.plist. Another area is that most editors out there treat UFO as an afterthought and export whatever they want without any regard for round-tripping.
    4. Use shared feature files and include(...) them in the UFOs. Makes it quite manageable actually with plain git.
    5. Hinting data is a nightmare, stuff in data/ is tossed by most UFO im-/exporters and you must not commit the removals. Always rely on autohinting!
chrissimpkins commented 6 years ago

ufonormalizer

https://github.com/unified-font-object/ufoNormalizer FFR

What does it do? The docs are... sparse :)

We're talking about designers here, some of which are surprisingly CLI-savvy, the rest of which revolt in horror at the mere thought

Have any of your designers tried the git GUI applications? Git Kraken is fantastic. I use it all the time. It is rare that I need to drop to the command line for routine work. It would give them visual representation of their branches vs. other remote branches and a set of buttons to click for branching/pulls/pushes. And notifies, adds support for simple conflict resolution when there is a merge conflict in a way that is less daunting than command line git. I'm sure that there are free tools that do the same, but I haven't tried so can't recommend.

madig commented 6 years ago

Have any of your designers tried the git GUI applications?

Some do, me and a few others like to use Fork. One of the major problems with e.g. Glyphs in the workflow is that both Glyphs and glyphsLib are imperfect and designers regularly get swamped with humongous amounts of git diffs for innocuous changes depending on glyphsLib version, Glyphs version and moonphase. Then there's stuff generated by tools, e.g. things that work on hinting. What if you did many changes and did not commit in atomic steps? Are you pro enough yet to understand every change done to a UFO? Can you build and diff the textual changes to a .glif in your head? Many throw their hands up and just commit everything and I can understand that. Oh, hello there merge conflict!

Font development is and remains a major PITA because the (graphical) tooling just isn't there yet. And that's before you start dealing with application quirks (hello Office for Mac!)

What does it do? The docs are... sparse :)

Many things, don't know all of them, but formatting XML in one way and sorting keys are among them iirc. We use it for round-tripping to Glyphs and back due to reordering of keys, whitespace, ...

anthrotype commented 6 years ago

let's unilaterally and opportunistically declare that whatever is the output of fontTools.ufoLib is the "normalized" representation of a UFO data, and be done with ufonormalizers. Just sayin'

madig commented 6 years ago

What to do about editors that reorder keys? Does ufoLib order them back?

anthrotype commented 6 years ago

what order are you talking about? the property list dictionary are always sorted alphabetically.

anthrotype commented 6 years ago

and plist arrays area inherently ordered, if authoring tools change their order, that's a bug

madig commented 6 years ago

Hm. @moyogo, @belluzj: is there something specific in our Glyphs/Fontlab round-tripping that requires normalization that ufoLib does not do by itself?

anthrotype commented 6 years ago

tabs

chrissimpkins commented 6 years ago

let's unilaterally and opportunistically declare that whatever is the output of fontTools.ufoLib is the "normalized" representation of a UFO data, and be done with ufonormalizers. Just sayin'

Maybe a pre-commit hook for that?

madig commented 6 years ago

Worth a try I guess? A script that opens all UFOs, loads everything/marks everything dirty without actually doing anything and writes it back. Can even run on the CI first.

chrissimpkins commented 6 years ago

Are you pro enough yet to understand every change done to a UFO? Can you build and diff the textual changes to a .glif in your head?

No one will ever be able to do this. But that is where the tooling like the browser visual diff tool (can't remember name) should come in to play. There needs to be a visual diff level of automation because sometimes changes in the source text do not lead to visual differences, or only do at certain sizes. I have yet to find a tool that allows for simple visual diffs that include size specific rendering with TT instruction sets considered. That is a tool that I would love to see. I don't know how to make it...

madig commented 6 years ago

Yes, and it's why there's still years of effort ahead of us :)

chrissimpkins commented 6 years ago

Part of what makes type source special :) It is not simple to interpret for humans, even in text format. And then you build with it and pile on post compilation modifications as icing on the cake.