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

Many simple checks versus fewer larger checks #4735

Open
felipesanches opened this issue May 22, 2024 · 5 comments
Open

Many simple checks versus fewer larger checks #4735

felipesanches opened this issue May 22, 2024 · 5 comments

Comments

@felipesanches
Copy link
Collaborator

@simoncozens made a suggestion at #4418 (comment) and then also hinted at the idea when posting #4729 (comment)

When we think of some new thing which needs checking, our response is normally to add another check for it. This means we have a large number of small, discrete checks, which individually are easy to understand and reason about but which don't necessarily "communicate" with one another. I would prefer to move towards a smaller number of checks which tell you everything about a particular topic - i.e. license URLS in all the different places they can appear - because then they can ensure consistency together.

@felipesanches
Copy link
Collaborator Author

I agree, in principle, but I fear clarity of rationale for each check validation may be lost.

There are some checks that really validate the same kind of thing in multiple places, with the exact same rationale. For those it would be fine to clump everything in a single check.

But there are also small checks that have different reasoning each, such as the many aspects that we validate on different fields of the METADATA.pb file. Clumping those together would make it more complicated to convey to users what's the reasoning for each specific thing being checked. And it would also result in multiple "proposed" links to issue tracker entries.

@felipesanches
Copy link
Collaborator Author

I suggest we start with one initial proposal of clumping together a given set of checks, and we see how we feel about it. If we like the result, we can move on to refactor additional sets of checks.

@simoncozens, which ones would you like to propose clumping together at first?

@simoncozens
Copy link
Collaborator

Description/article. It's a GF-only thing so it only affects our process, and it's not as sprawling as metadata.pb.

@m4rc1e
Copy link
Collaborator

m4rc1e commented May 22, 2024

I'd personally like to do away with the little checks tbh. However, this isn't a hill I'm willing to die on.

I'd much prefer it is there was a single check for METADATA.pb that simply compares the current metadata.pb against a generated one, since we shouldn't be editing these by hand anyway (apart from the designer name, stroke etc).

felipesanches pushed a commit that referenced this issue May 28, 2024
On the Google Fonts profile:
- com.google.fonts/check/metadata/copyright_max_length
- com.google.fonts/check/metadata/nameid/copyright
- com.google.fonts/check/metadata/valid_copyright
- com.google.fonts/check/name/copyright_length

These checks have all been merged into a new one to replace them:
com.google.fonts/check/font_copyright

(PR #4748 / issue #4735)
felipesanches pushed a commit that referenced this issue May 28, 2024
On the Google Fonts profile:
- com.google.fonts/check/metadata/copyright_max_length
- com.google.fonts/check/metadata/nameid/copyright
- com.google.fonts/check/metadata/valid_copyright
- com.google.fonts/check/name/copyright_length

These checks have all been merged into a new one to replace them:
com.google.fonts/check/font_copyright

(PR #4748 / issue #4735)
@simoncozens
Copy link
Collaborator

Just to keep track of a few checks I have been merging as I look at porting them:

  • DSIG is now just an unwanted table, com.google.fonts/check/dsig moves into com.google.fonts/check/unwanted_tables
  • com.google.fonts/check/unique_glyphnames can be checked at the same time as com.google.fonts/check/valid_glyphnames
  • com.google.fonts/check/metadata/parses, com.google.fonts/check/metadata/designer_values, com.google.fonts/check/metadata/date_added, com.google.fonts/check/metadata/category, com.google.fonts/check/metadata/menu_and_latin, com.google.fonts/check/metadata/subsets_order, com.google.fonts/check/metadata/single_cjk_subset are all standalone METADATA.pb validation checks, can all be done at the same time.
  • com.google.fonts/check/varfont/regular_wght_coord, com.google.fonts/check/varfont/regular_wdth_coord, com.google.fonts/check/varfont/regular_slnt_coord, com.google.fonts/check/varfont/regular_ital_coord, com.google.fonts/check/varfont/regular_opsz_coord can all be checked at the same time with a dictionary of regular coordinate values; similarly for the range checks in opentype/fvar.py.

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

No branches or pull requests

3 participants