search

fonttools / fontbakery

🧁 A font quality assurance tool for everyone
https://fontbakery.readthedocs.io
Apache License 2.0
553 stars 103 forks source link

Split checks from profiles #3188

Closed simoncozens closed 8 months ago

simoncozens commented 3 years ago

Last time I tried to add a profile (#3168), I was told that we shouldn't proliferate profiles and that the checks should find their way into other profiles. I agree with this, it's a good idea! The problem is that the checks need to be "put" somewhere and right now the only place we have to put them is a package called fontbakery.profiles. Now we are talking about the GRAD check (#3187). I'm happy to write a check but I don't want to have to fight over which profile it should go in. These are separate concepts.

In fact, right now, we have some files in fontbakery.profiles which describe profiles, some which describe checks, and some which contain both checks and profiles. This is a bit crazy.

I would like to see a package called fontbakery.checks which contains all the checking code, with fontbakery.profiles reserved for actual top-level profile. Things in fontbakery.profiles should just load up the checks that they need and return a list. This is cleaner and it also means that the list of profiles can be automatically loaded from all the modules in the package.

Eventually the fontbakery.profiles stuff can probably go away and become a bunch of JSON files.

graphicore commented 3 years ago

We could have a profile for new contributions, to make it easier. Also maybe a profile for things that don't fit quite into existing buckets. When a trend establishes we could create a new profile that fit that.

simoncozens commented 3 years ago

If the answer to this problem is to create a profile containing checks that are not members of a profile, that really convinces me that we need to separate checks from profiles...

chrissimpkins commented 3 years ago

If we dive into a significant refactor of the profile / check structure, it would be great to see the development of an external configuration file with upstream and custom local test grouping support as part of this effort.

fontbakery.toml
[fontbakery.profiles.universal]
exclude = [
  "skipped/universal/check/1",
  "skipped/universal/check/2",
]

[fontbakery.checks]
include = [
  "some/nonuniversal/upstream/check/1",
]

[local.profiles]
paths = [
  "path/to/custom/localprofile/1",
  "path/to/custom/localprofile/2",
]

Eventually the fontbakery.profiles stuff can probably go away and become a bunch of JSON files.

We may be thinking along the same lines. And what I show above may be the new "profile" structure similar to what you suggest. If the check implementation is pulled out of the check grouping definition, it should be possible to define groups of tests through some structured data format that could live in either upstream (for the fontbakery maintained check groups) or downstream (to support expansion with custom checks in new check groups) locations. If the definition file is meant to support manual edits, my own preference is to use YAML or TOML rather than JSON. Beyond being more human friendly, you also get comment support which can be helpful for annotation. It is important that any change maintains support for downstream check grouping definitions that include a mix of upstream and downstream checks.

simoncozens commented 3 years ago

I love that idea, as it would also allow individual projects to pass configuration details to the checks. This would clean up eg the shaping check, by eliminating hard-coded data file paths.

graphicore commented 3 years ago

If the answer to this problem is to create a profile containing checks that are not members of a profile, that really convinces me that we need to separate checks from profiles...

As I explained to you before, the profile is defining the namespace in which the check is supposed to be executed. FontBakery uses Dependency Injection, so you don't have to call the check yourself. You define a check, and by the names of the arguments FontBakery decides how often to run the check, e.g. repeatedly for each ttFont, and what type the arguments are.

The namespace is mainly made up of @condition elements which are used to process and prepare the original arguments to FontBakery.

Hence, a profile is nothing else than defining a collection of checks and the dependencies required to run those checks. As such I don't see a fundamental difference to a normal python module, and in fact profile definition piggybacks much on the module system.

I'm happy to write a check but I don't want to have to fight over which profile it should go in. These are separate concepts.

I can't believe the problem is to mark up a module as a profile, it seems more like there's a political issue and an overloading of the profile as more than just a description of dependencies.

graphicore commented 3 years ago

Trying to follow you here.

Last time I tried to add a profile (#3168), I was told that we shouldn't proliferate profiles and that the checks should find their way into other profiles. I agree with this, it's a good idea!

Not sure if I agree this is a good idea TBH.

The problem is that the checks need to be "put" somewhere and right now the only place we have to put them is a package called fontbakery.profiles. Now we are talking about the GRAD check (#3187). I'm happy to write a check but I don't want to have to fight over which profile it should go in. These are separate concepts.

Yes, the concepts are different: the profile is the container of the check; it defines the namespace available to the check. You can organize it differently, but you can't remove the namespace thing, it's important that we know what the arguments of a check are supposed to be.

In fact, right now, we have some files in fontbakery.profiles which describe profiles, some which describe checks, and some which contain both checks and profiles. This is a bit crazy.

Not true. At least, if true, it would be a bug. But the minimal profile definition is usually like this: from fontbakery.fonts_profile import profile_factory # NOQA pylint: disable=unused-import

I would like to see a package called fontbakery.checks which contains all the checking code, with fontbakery.profiles reserved for actual top-level profile.

"top-level profile" introduces a new concept, never heard of it before this. I think your fontbakery.checks is actually the current fontbakery.profiles. If so, you suggest $ mv profile checks.

Things in fontbakery.profiles should just load up the checks that they need and return a list.

Now you use the new concept and impose it onto the old one. That's confusing.

It must also load up the @conditions!

This will be very specific for e.g. the fonts_profile. I would not accept a solution that removes the general ability to create a generic profile with another namespace/purpose etc. But, it's cool to improve the tooling for the fonts_profile which is the environment for the vast majority of checks and I would hope to see a generally useful approach.

This is cleaner and it also means that the list of profiles can be automatically loaded from all the modules in the package.

Please explain. Why would all the modules in the package want to load the list of profiles and why can't they do it right now?

Eventually the fontbakery.profiles stuff can probably go away and become a bunch of JSON files.

You could just go ahead and implement this in a profile and if it's useful we can generalize it and remove friction.

simoncozens commented 3 years ago

I'll try again. I think there is an important difference here.

We already have a distinction between what I am calling "top-level profiles" and other profiles: a top-level profile is a profile that we consider important enough to have a fontbakery.commands.check_... file for, and that we promote as having a fontbakery subcommand.

The fontbakery.commands.check_... files are largely redundant. They're basically identical. The only reason we need them is to define subcommands for the front-end CLI. But the profile file "knew" if it was supposed to be top-level or not, the associated fontbakery.commands.check_... file wouldn't need to exist at all. As you mention in https://github.com/googlefonts/fontbakery-ui/issues/15, if we put TOPLEVEL = True or something in the profile file of the ones we consider important, the font-end CLI (and fontbakery-ui) can derive the list of check-... subcommands by walking through the list of profiles and picking out the top-level ones.

I'm not suggesting that we do this, because I think there is a better approach which I'll explain below, but I want to show that there are already two different kinds of "thing" in fontbakery.profile: promoted, top-level profiles; and non-promoted profiles.

As well as those, we have a bunch of other stuff in fontbakery.profiles. Some of them aren't profiles at all. fontbakery.profiles.googlefonts_conditions is not a profile. fontbakery.profiles.shared_conditions is a profile, bizarrely, but I don't think it should be. But they aren't really what I'm talking about.

I'm talking about stuff like fontbakery.profiles.dsig. Yes, technically that is a profile. But the primary purpose of that file is not to define a list of checks that the user can use. The primary purpose, really, is to define the com.google.fonts/check/dsig check, so that this check can be imported in other profiles - for example, the opentype profile. If I want to show a list of profiles in fontbakery-ui that the user should choose, should I include fontbakery.profiles.dsig? No, of course not; it's just a check.

The opentype profile, on the other hand, does not define any checks. It is purely a list of checks. These two profiles - opentype and dsig - have a very different nature.

But the adobefonts profile does both! It describes a list of checks to perform, and it also defines the definition of some of those checks.

I think these two roles of the "profile" concept - defining which checks we want to perform and defining how to perform checks - should be separated.Here is my alternative architecture:

This is really useful because now profiles can mix and match - they can choose checks from a "pool" of checks. The fontbakery check runner loads up all the fontbakery.checks.* files and registers each function by its check ID. Then it asks the profile for the list of checks that it wants to perform.

Once we have done this, all we should have in fontbakery.profiles.* are basically files containing lists of check names. This can be simplified and data-driven.

I understand your point about the namespacing of arguments. It's a very clever idea. But I don't think it's all that big a deal. We have the argument names for that. ttFont is going to be a fontTools.ttLib.TTFont object in all profiles, or else it's going to be very surprising for the programmer. If you have an argument called font, sure, that's ambiguous. But we don't, and we shouldn't, so it's not a problem.

graphicore commented 3 years ago

We already have a distinction between what I am calling "top-level profiles" and other profiles: a top-level profile is a profile that we consider important enough to have a fontbakery.commands.check_... file for, and that we promote as having a fontbakery subcommand.

All other profiles can be used with fontbakery.commands.check_profile. A list ['googlefonts', 'adobefonts',notofonts, ...]could also do the promotion.

As well as those, we have a bunch of other stuff in fontbakery.profiles. Some of them aren't profiles at all. fontbakery.profiles.googlefonts_conditions is not a profile. fontbakery.profiles.shared_conditions is a profile, bizarrely, but I don't think it should be. But they aren't really what I'm talking about.

I was never really happy about those two, now I know why, their existence stimulates your argument.

Most of this, i.e. reorganizing stuff, sounds acceptable but the last part. We should not bind the check runner to a certain namespace, in this case the one of fonts_profile. But this should be doable, just don't hard code that fontbakery.checks.* is the only "pool" of checks. Maybe a profile can define what pool of checks it intends to use.

The way I see it, fontbakery.checkswill be a pool that is specific for a namespace, the one that is associated now with the fonts_profile and that is for our case ubiquitous. If I want or need another namespace, those checks must go somewhere else. Maybe we can make future contributions easier than what you have experienced when trying to contribute something, but my gut feeling is, making generic things will be a lot harder.

If we don't pay attention now, it may even shut that door completely.

I understand your point about the namespacing of arguments. It's a very clever idea. But I don't think it's all that big a deal. We have the argument names for that. ttFont is going to be a fontTools.ttLib.TTFont object in all profiles, or else it's going to be very surprising for the programmer.

Before – we could use check_runner for all kinds of things. After – it's bound to this, and only this, purpose. I dislike this specialization wholehearted, though, I believe it does not have to be, and we can reorganize and still have a check runner that is agnostic of the specifics of how we do things currently. Having this separation of concerns keeps some order in the code base, and even if there's not another big use case right now, it helps thinking of it, as if there would be one.

This is really useful because now profiles can mix and match - they can choose checks from a "pool" of checks. The fontbakery check runner loads up all the fontbakery.checks.* files and registers each function by its check ID. Then it asks the profile for the list of checks that it wants to perform.

We can do exactly this already, there can be a profile that contains all checks, we already have command line arguments that do this kind of selection. You could make this happen "data driven" in 15 minutes using a JSON file as list of checks.

If you have an argument called font, sure, that's ambiguous. But we don't, and we shouldn't, so it's not a problem.

:rofl: we literally have font it's e.g. the argument of ttFont, so it is a problem and it will be one again because everything that can go wrong will go wrong.

simoncozens commented 3 years ago

We should not bind the check runner to a certain namespace, in this case the one of fonts_profile. But this should be doable, just don't hard code that fontbakery.checks.* is the only "pool" of checks. Maybe a profile can define what pool of checks it intends to use.

I agree with this. And in fact it's the only way to do custom profiles out-of-tree - they would have to define their own pool. That's fine.

Before – we could use check_runner for all kinds of things. After – it's bound to this, and only this, purpose.

I understand why it's a good idea to make software implementations as generic as possible. But at some point you have to specialize. This project is called fontbakery; a certain amount of specialisation is expected. :-) We've talked about using fontbakery to check things which are not fonts. We need to allow that. But when we talked about that, @felipesanches wanted those checks would live in the same profiles as the font checks:

In general I prefer to avoid creating new profiles. Things should fit some of the existing profiles such as universal or googlefonts... In the future we might even make the profiles smart to distinguish whether source or binary checks should be performed, based on the types of files provided on the command line.

So you want to namespace profiles so that different profiles can define different "kind of things" that they test, and Felipe wants profiles to be able to test many different kinds of things!

graphicore commented 3 years ago

(Maybe we must make namespace a literal thing, we could mix stuff, detect collisions ... :thinking: )

I understand why it's a good idea to make software implementations as generic as possible. But at some point you have to specialize. This project is called fontbakery; a certain amount of specialisation is expected. :-)

Maybe, but it's neither necessary nor do we have to. Instead of specializing check_runner, we should specialize fonts_profile. A profile is nothing else than specialization for a purpose and providing a generic interface for check_runner.

So you want to namespace profiles so that different profiles can define different "kind of things" that they test, and Felipe wants profiles to be able to test many different kinds of things!

I can't follow this. I don't think it compares well. Felipe wants the fonts_profile to do things, I want the check_runner to be unaware of those things.

I also want to have attached to each check the environment information (namespace) it expects, but a profile already does that. The pool, by convention, could provide that environment information.

simoncozens commented 8 months ago

This happened!

bobh0303 commented 8 months ago

This happened!

Yes! And the new profile stuff is amazing.

Question: Should old profiles still work, or is it easy to make them work?

I'm getting an error with ours:

ImportError: cannot import name 'get_module_profile' from 'fontbakery.profile'

but I don't know whether they should work (and we have an error) or they aren't expected to work.

I know it isn't that hard to write a replacement, but for backwards compatibility with older fontbakery.

simoncozens commented 8 months ago

Old profiles will not work - profiles are now pure data, not code, and are much, much simpler, but I'm sorry about the lack of backwards compatibility.

Please look at the in-built profiles for examples of how profiles now look. See also the new Writing Profiles guide.

bobh0303 commented 8 months ago

As I suspected. thx.