Initial support for checking UFOs #1736
Conversation
First test runs ufolint on *ufo.
|
So there's good news and bad news. 👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there. 😕 The bad news is that it appears that one or more commits were authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request. Note to project maintainer: This is a terminal state, meaning the |
|
Eh... I'm submitting this PR from work, that's probably why googlebot mixes something up. |
|
you need to add your dama email address to your @madig github profile |
| yield FAIL, ("ufolint failed the UFO source. Output follows :" | ||
| "\n\n{}\n").format(e.output) | ||
| except OSError: | ||
| yield ERROR, "ufolint is not available!" |
There was a problem hiding this comment.
shouldn't this ufolint be added as a dependency?
by the way, what does it actually do?
There was a problem hiding this comment.
Counting lints in a UFO? Haven't read the source closely, but it will e.g. complain if the XML is malformed.
There was a problem hiding this comment.
yes, I was wondering about that as well.
|
In the past we had some ufo source checks but they were removed in order to refocus our development efforts. If you're willing to add more ufo checks, then I'm not in principle against doing so. Having said that, I see that your first check simply hooks up an external tool (an UFO linter). I would like to postpone merging this PR until when we get at least a couple more checks here. What do you think, @davelab6 ? Maybe @graphicore and @m4rc1e have thoughts to share here as well? I think this kind of thing can be very useful for Marc, for instance. |
|
@madig would you be OK with working for a while (a few days? a week? a month?) on this branch keeping this PR open until we decide it is good enough to justify merging into master? Or would you rather commit early to a set of new checks to be added (and debugged) in the near future? |
|
I'm fine with postponing, this is as much a proof of concept as it is a PR :) I want to do some company-internal checks here and work out what's useful and what isn't. |
|
Great! Feel free to make them public in this PR (by pushing more commits to the same branch) and we'll likely have them in FB soon. If it is truly useful internally, then there's a good chance it may also be useful here and we'd love to gather such checks. I looked at Travis and noticed that this initial commit breaks a few things. As it seems you're warming up for collaboration, I would be happy to help you figure out these things we deal with on a daily-basis while developing FontBakery. For instance, any time one opens a pull request, a corresponding automated test-suite runs on Travis. You can see it at: https://travis-ci.org/googlefonts/fontbakery In particular, the latest commit resulted in a build-fail with the logs available here: https://travis-ci.org/googlefonts/fontbakery/builds/355351088 It would also be good to always add code-tests to ensure all code-paths in a font-check are behaving as expected. You can see several examples of this approach at https://github.com/googlefonts/fontbakery/blob/master/Lib/fontbakery/specifications/googlefonts_test.py |
|
I know Travis, but why isn't it hooked up as a check here in the PR next to "cla/google"? Will unbreak the tests :D I also added my work email to my account, can you trigger googlebot? |
|
|
||
|
|
||
| @register_check | ||
| @check(id='com.google.ufos/check/001', priority=CRITICAL) |
There was a problem hiding this comment.
A check id does not really need to end in a number. We've been doing it that way, but perhaps we could use this as an opportunity to experimenting with doing it differently. Maybe this should end in /ufolint instead.
Also, you may prefer to identify the original contribution by using a different choice of reverse domain instead of com.google.
I also don't see a point in .ufoshere. If @davelab6 brings up some ufo-checking need from the Google Fonts project, for detecting some "foo" problem, I would then create a new check in this spec called something like com.google.fonts/check/foo and the resulting command line invocation to run this single check would then be:
fontbakery check-ufo-sources -c com.google.fonts/check/foo --verbose
For Dalton Maag contributions for checking some hypothetical "example" issue on UFO sources, then I'd suggest adopting an id such as: com.daltonmaag/check/example
Maybe it could be com.domain/check/ufo/something to make the fact we're dealing with UFOs explicit on the id. But I am not sure about that, yet.
@graphicore do you agree with my considerations here? Or would have other suggestions? This is definitely a work in progress.
There was a problem hiding this comment.
And maybe at some point we could consider adding a command-line attribute like -d domain to run only checks from a given domain. In that case, running only the Dalton Maag contributed UFO checks could work like this:
fontbakery check-ufo-sources -d com.daltonmaag --verbose
(possibly allowing multiple -d and -c flags in a single command to cherry-pick what a user wants...)
But I'm just speculating here. Not sure yet what we'll actually do.
There was a problem hiding this comment.
(How about pytest's -k switch:
-k EXPRESSION only run tests which match the given substring
expression. An expression is a python evaluatable
expression where all names are substring-matched
against test names and their parent classes. Example:
-k 'test_method or test_other' matches all test
functions and classes whose name contains
'test_method' or 'test_other', while -k 'not
test_method' matches those that don't contain
'test_method' in their names. Additionally keywords
are matched to classes and functions containing extra
names in their 'extra_keyword_matches' set, as well as
functions which have names assigned directly to them.
)
There was a problem hiding this comment.
That looks powerful, but also a bit of over-engineering. I'd probably avoid that complexity.
|
Can't replicate the test failure locally... |
|
I tend to think that UFO (and .glyphs) checking belongs in an editor extension rather than fontbakery, which has recently been scoped to checking TTFs. However, if the fontbakery architecture is appealing to DaMa for running source checks, then having a profile as Felipe said for UFO would be great! Almost all Google Fonts contributors are supplying .glyphs source files now, so we would benefit from a GlyphsLib based source checker too, and Marc has made one as a Glyphs in-app script (with a small UI) |
|
I think it is okay with us if you decide that the UFO checking I do (still drafting!) is of no interest to the official fontbakery scope -- we can just as well make it an internally distributed module that gets loaded by check-spec and contribute just the parts to make that use-case work. I suppose an editor extension makes sense, but only if the editor uses UFO as a native format. We round-trip from Fontlab to UFO with an internal script, which sometimes does unnecessary things -- hence the external checks we want to do on commits. |
|
I am trying to accommodate/incentivise you to share your UFO checking,
since we have some (but a minority) of project using UFO source files and
will see mutual benefits of working together :)
|
|
Oh, okay, no need to try, I'm motivated already ;) |
A font bakery editor extension could be an option. The font bakery check runner is rather light weight and an editor specific reporter could be created.
Another repo then for none-TTF specs? Maybe I should spent some time to figure out and write down how third parties can best write their own specs, how checks can be shared and so on. Would also be a good thing to point to at TypoLabs. |
|
I agree with Lasse on his comments. But if you'll write fontbakery-based checks I'd prefer to host them all in a single repo (here) to reduce potential segmentation. I pretty much would prefer to keep it all together in a "single universal font testing system" ;-) |
|
@madig please add "check-ufo-sources" to the expected list of subcommands at test_list_subcommands_has_all_scripts |
|
Please, also consider creating a new |
|
Okay, I was just surprised I can't reproduce the failure on my work machine... Will do. |
|
Once we get that minimal setup I'll be happy to merge soon (and often), as long as the code looks good and travis is kept green ;-) |
You may be running pytest on just the specifications subfolder and because of that the rest of the checks are not run on your machine. (I am guessing) |
I see your point here and I agree that consequently contributing to this should be straight forward. I have some remarks though.
|
|
In general I agree about minimizing dependency trees, but, FB is a kind of
'sink' so I am not very strong on this. Up to the users :)
|
|
Not all dependencies have to be installed, some parts can be unavailable unless their dependencies are installed. |
|
@felipesanches when I run And please hook up Travis to the check-list thing below so Travis status displays there without me having to open a separate tab :) |
| @@ -0,0 +1,29 @@ | |||
| # -*- coding: utf-8 -*- | |||
There was a problem hiding this comment.
could you please rename this file so that its name is identical to the name of the script it tests, except for the _test suffix?
I mean to call it ufo_sources_test.pyinstead of ufo_source_test.py in order to match Lib/fontbakery/specifications/ufo_sources.py
To match name of tested file plus _test suffix.
Makes flake8 and me happy.
| except OSError: | ||
| yield ERROR, "ufolint is not available!" | ||
|
|
||
| yield PASS, "ufolint passed the UFO source." |
There was a problem hiding this comment.
you need a boolean initialized as failed=False then make it True in the exception handlers above and then do "if not failed yield pass" here.
There was a problem hiding this comment.
that's a general pattern used throughout our codebase, actually
There was a problem hiding this comment.
Oh, why not use else: appended to the try block?
There was a problem hiding this comment.
the else branch runs when there is no exception, before the finally clause. That is its primary purpose. Here there is no finally so it doesn't make any difference
There was a problem hiding this comment.
Another use of else is to do something only when no exception occurs and to not do that when exceptions are handled. In this case there is a yield in the except: branches, not a return, so technically Nikolaus is correct: if one where to call next() on the iterator returned by this function after a FAIL or an ERROR, it will surprisingly continue execution and meet a yield PASS. So yes, it would make sense to protect it with an else
| import defcon | ||
|
|
||
| unnecessary_fields = [] | ||
| ufo = defcon.Font(font) |
There was a problem hiding this comment.
This is repeated in more than a single check. Make it a condition so that it can be reused (and probably save CPU time and memory)
You can look at specifications/googlefonts.py for examples such as this:
https://github.com/googlefonts/fontbakery/blob/fc9a4e68e5f002fe5df805b5fc6696b6bd1732cd/Lib/fontbakery/specifications/googlefonts.py#L78-L82
To use it, you simply pass the condition name as an argument to the check implementation.
There was a problem hiding this comment.
You don't need to use conditions=['ufo'] in this case (just like in the ttFont, it is also a special case). But in general, for other conditions you may want to do this:
The result of doing so is that if registered_vendor_ids is not available ("if not True"), then the check will be automatically skipped and this will be logged in the fontbakery summary report.
| com_daltonmaag_check_recommended_fields as check) | ||
| ufo, ufo_path = empty_ufo_font | ||
|
|
||
| print('Test FAIL with empty UFO.') |
| import defcon | ||
| import pytest | ||
|
|
||
| from fontbakery.checkrunner import (DEBUG, ENDCHECK, ERROR, FAIL, INFO, PASS, |
There was a problem hiding this comment.
Remove ENDCHECK here. Travis complains because this was not used at all.
At https://travis-ci.org/googlefonts/fontbakery/builds/355967025 you see:
************* Module fontbakery.specifications.ufo_sources_test
W: 8, 0: Unused ENDCHECK imported from fontbakery.checkrunner (unused-import)
|
According to https://travis-ci.org/googlefonts/fontbakery/builds/355967025 you are missing a "check-ufo-sources" subcommand. I think what this actually means is that in order to fix the |
|
But |
|
You can define optional dependencies with extras_require like in: robotools/defcon#177 |
did you make it an executablescript with |
|
I want to merge this PR, but there are 2 code-tests failing right now:
@madig Could you quickly fix these so that I can merge this PR? Detailed Travis log listing these two problems at: |
.travis.yml
Outdated
| @@ -19,6 +19,7 @@ install: | |||
| - pip install pytest | |||
| - pip install flake8 | |||
| - pip install pylint | |||
| - pip install ufodiff | |||
There was a problem hiding this comment.
As we get an ERROR on the com_daltonmaag_check_ufolint test on Travis, it probably means we need to install ufolint here as well.
There was a problem hiding this comment.
| print('Test PASS with empty UFO.') | ||
| c = list(check(ufo_path)) | ||
| status, _ = c[-1] | ||
| assert status == PASS |
There was a problem hiding this comment.
Does ufolint really PASS with an empty UFO? Shouldn't it complain about it being empty? And also all other problems that such a degenerated case would likely trigger?
There was a problem hiding this comment.
I installed the wrong package because the cross-activity on GH distracted me :D
I think ufolint only does a bare minimum checking of technicalities. That's why there are more checks to be made ;)
There was a problem hiding this comment.
greeeeeeeeeeeeeeeeeeeeeeen
|
Thanks a lot, @madig ! We appreciate the work you're doing ;-) Feel free to open another PR if you have further contribs to submit and we'll review those as well. cheers! |
|
Damn. I forgot to pull Unicode ranges from unnecessary. Those actually have to be set by hand, ufo2ft won't do that, on purpose. |
First test runs ufolint on *.ufo.