gftools icon indicating copy to clipboard operation
gftools copied to clipboard

Rethinking the gftools/fontbakery boundary

Open simoncozens opened this issue 3 years ago • 10 comments

This is inspired by something @m4rc1e said elsewhere:

When we finally get gftools imported into fb as a dependency, we'll have easy access to the GF glyph sets/encodings.

This creates a situation where:

  • gftools depends on fontbakery for running QA tests (gftools qa)
  • fontbakery depends on gftools for data

A mutual dependency is obviously a Bad Thing from a practical point of view. But from an architectural point of view, it's actually a Good Thing because it forces us to re-evaluate the boundary between the two packages!

Let's step back and think about what is in each package. I'm reminded that @graphicore has often argued (see here amongst other places) that fontbakery is a very generic check runner which provides cool things like dependency injection and profiles and so on, and that nothing should happen to the fontbakery check runner that makes it more font-specific. And that's good. I agree!

But it means we currently have:

fontbakery

  • generic check runner
  • knowledge about diagnosing font problems
  • some miscellaneous font metadata (e.g. fontbakery.constants includes style names, licensing text, name/encoding IDs, GF Latin core glyph list...)

gftools

  • knowledge about how to fix font problems (often the same problems diagnosed by fontbakery)
  • assorted font-related tools
  • some miscellaneous font metadata (e.g. gftools.util.styles includes style names, gftools.constants includes name/encoding IDs, gftools.encodings includes GF Latin core glyph list...)

How can we arrange these different things to encourage code reuse and reduce mutual dependencies?

Here's my suggestion. The suggested package names are terrible but you'll get the idea:

thebakery

  • generic check runner

gfdata

  • a "data-only" module similar to opentypespec which contains all the font metadata we use.

fontdoctor

(requires gfdata)

  • knowledge about how to diagnose font problems.
  • knowledge about how to fix font problems.

fontbakery

(requires fontdoctor and thebakery)

  • Font QA engine (diagnosis/testing only)

gftools

(requires fontbakery, fontdoctor and gfdata)

  • assorted font-related tools
  • front-end CLI utilities to call fontbakery
  • front-end CLI utilities to call fontdoctor fixes

simoncozens avatar Aug 05 '21 08:08 simoncozens

"GF Latin core glyph list" is currently included in fontbakery.constants exactly because we did not want to include the entire gftools as a dependency

felipesanches avatar Aug 05 '21 08:08 felipesanches

I think I'm going to put my refactoring work in #397 on hold until we've got this issue sorted. It may make sense to talk about it in tomorrow's meeting. Overall, I like Simon's dependency tree but we could make some tweaks which are worth discussing.

m4rc1e avatar Aug 05 '21 08:08 m4rc1e

"GF Latin core glyph list" is currently included in fontbakery.constants exactly because we did not want to include the entire gftools as a dependency

Right. If two modules want to have access to the same thing (data, in this case), you can either (a) make one module a dependency of the other, (b) duplicate the thing, or (c) refactor the shared thing into a third module that both can use. You don't want to do (a) (right!), so you did (b). But the idea of Don't Repeat Yourself suggests that (c) is a better approach.

simoncozens avatar Aug 05 '21 08:08 simoncozens

sounds good!

felipesanches avatar Aug 05 '21 08:08 felipesanches

@graphicore has often argued (see here amongst other places) that fontbakery is a very generic check runner which provides cool things like dependency injection and profiles and so on, and that nothing should happen to the fontbakery check runner that makes it more font-specific. And that's good. I agree!

Kindly I disagree, since if its constitutionally a fully generic check runner, that begs the question why we didn't just use one of the 100s of existing fully generic check runners - and my answer to that question has been that we (will) have font specific needs that I want to be "batteries included" with.

If we refactor out thebakery, then I wonder if there are other check runners that will be comparable, and if we do some comparisons, are there things we can improve to be superlative? :)

davelab6 avatar Aug 05 '21 16:08 davelab6

But it means we currently have:

  1. fontbakery
  2. gftools

How can we arrange these different things to encourage code reuse and reduce mutual dependencies?

I think there's a 3rd piece of this - https://github.com/googlefonts/gf-docs - and since in a way the googlefonts profile is a coded version of the GF Spec, which both includes a lot of technical super detail that isn't in the doc, and lacks a lot of stuff which either can't easily be coded or just hasn't been yet, then, I'd like to advocate that any re-architecting include the human readable docs.

Given topically-unrelated efforts by related people - to make better structured docs for the font format spec, OT features, fontbakery check dogma, etc - perhaps there's something we can do to deduplicate and pattern things so there's a common approach that makes contribution skills more transferrable

davelab6 avatar Aug 05 '21 17:08 davelab6

I've started drafting a UML diagram for this issue, https://drive.google.com/file/d/1C84Ghabaz7hI1bHlnhCbcOWmhA3O2WQl/view?usp=sharing. Let's work on it in today's meeting.

m4rc1e avatar Aug 06 '21 10:08 m4rc1e

Kindly I disagree, since if its constitutionally a fully generic check runner, that begs the question why we didn't just use one of the 100s of existing fully generic check runners - and my answer to that question has been that we (will) have font specific needs that I want to be "batteries included" with.

If we refactor out thebakery, then I wonder if there are other check runners that will be comparable, and if we do some comparisons, are there things we can improve to be superlative? :)

I looked at unit testing tools before writing the check runner, good luck finding a better fit. (Doesn't mean there isn't room for improvements.) I'd like to see the comparison.

Batteries included and separation of concerns should not be contradictory. I think of the "generic" property of the check runner as best practices approach to not mess things up and thereby go to spaghetti code hell — again.

If we refactor out thebakery, then I wonder if there are other check runners that will be comparable, and if we do some comparisons, are there things we can improve to be superlative? :)

It wouldn't be much of a refactor, so far concerns are separated (99%). I wonder, if we make thebakery: Are there other domains where it would be useful, finding a bigger community for it?

graphicore avatar Aug 17 '21 16:08 graphicore

I’m writing something now (a language/shaping tester) that would benefit from a standalone check runner...

simoncozens avatar Aug 17 '21 17:08 simoncozens

I've just solved a significant part of this puzzle!

Now all glyphset info is separated in a new reusable python module called glyphsets available at:

  • https://github.com/googlefonts/glyphsets
  • https://pypi.org/project/glyphsets/

Please read the details at: https://github.com/googlefonts/gftools/pull/453

felipesanches avatar Dec 17 '21 07:12 felipesanches