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

Rethink conditions #4507

Merged
merged 38 commits into from
Feb 14, 2024
Merged

Rethink conditions #4507

merged 38 commits into from
Feb 14, 2024

Conversation

simoncozens
Copy link
Collaborator

This is another major refactor to the internals, which greatly simplifies the checkrunner, profile and codetester modules.

The idea is that instead of the profile containing a dictionary of iterargs, the files to be tested are stored in a CheckRunContext object. The profile is just the checks to be run. The CheckRunContext object contains a list of testables, which are Font, Ufo, ... objects.

Now conditions become a lot simpler - whenever we want to "ask a question" about one of these files (is it TTF? is it bold? is it hosted on Google Fonts?), we simply call a (cached) property on the testable object. This means there is no need to arrange for arguments to be fed to the "conditions", and chain calls to them - a property on a Font can call another property on a Font just with standard Python method dispatch. This in turn eliminates around 50% of what checkrunner.py was doing.

A nice side effect of this is that codetesting.py no longer needs to use any internal methods of checkrunner. It just constructs a context, hands it to CheckRunner and looks at the results. There is a clean "mock object" syntax for fiddling with properties of the testables to exercise different codepaths.

(As a second nice side-effect, you can turn a single input file - say a .TTC - into multiple Font objects in your CheckRunContext, making it much easier to implement #2595.)

There are a few implications to this approach for profile authors. A condition is defined in almost the same way, but it must specify whether it is being added to Font (if it is asking a question about an individual testable), CheckRunContext (if it is asking a question about the test run as a whole, or needs to work with the collection of files being tested), etc., and it must take a single parameter, which is the testable object itself. Many of the simpler "standard" conditions are simple cached properties on the Font class.

Some things which were not really conditions but actually utility functions have been reworked as utility functions.

@simoncozens simoncozens force-pushed the rethink-conditions branch 5 times, most recently from d289fdf to b2133f9 Compare February 12, 2024 08:30
@simoncozens
Copy link
Collaborator Author

@felipesanches I think this is good to go!

@felipesanches
Copy link
Collaborator

Great, thanks!

@felipesanches
Copy link
Collaborator

felipesanches commented Feb 12, 2024

I like that checks are now first-class. Similarly to what you've done to code-testing (not requiring an explicit profile name, except when dealing with overriden checks), we should be also able to run a single check from the command-line without specifying a profile.

Also, I think the codebase now is in better shape for integration with font-editors, because it is simpler to invoke a check via simple python imports and method calls. (#4128 (comment))

@simoncozens
Copy link
Collaborator Author

Yeah, there are a few more enhancements we can do in the future but this puts us in a good shape to do them. And yes, totally agree that working with font editors should be much easier now.

@felipesanches felipesanches merged commit c4bf860 into main Feb 14, 2024
40 checks passed
@simoncozens simoncozens deleted the rethink-conditions branch February 14, 2024 11:21
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

Successfully merging this pull request may close these issues.

2 participants