Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rethinking the gftools/fontbakery boundary #398

Open
simoncozens opened this issue Aug 5, 2021 · 10 comments
Open

Rethinking the gftools/fontbakery boundary #398

simoncozens opened this issue Aug 5, 2021 · 10 comments

Comments

@simoncozens
Copy link
Contributor

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
@felipesanches
Copy link
Member

"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

@m4rc1e
Copy link
Collaborator

m4rc1e commented Aug 5, 2021

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.

@simoncozens
Copy link
Contributor Author

"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.

@felipesanches
Copy link
Member

sounds good!

@davelab6
Copy link
Member

davelab6 commented Aug 5, 2021

@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
Copy link
Member

davelab6 commented Aug 5, 2021

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

@m4rc1e
Copy link
Collaborator

m4rc1e commented Aug 6, 2021

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.

@graphicore
Copy link
Contributor

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?

@simoncozens
Copy link
Contributor Author

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

@felipesanches
Copy link
Member

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:

Please read the details at: #453

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants