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

[crater] Support for private fonts #1202

Merged
merged 1 commit into from
Jan 17, 2025
Merged

[crater] Support for private fonts #1202

merged 1 commit into from
Jan 17, 2025

Conversation

cmyr
Copy link
Member

@cmyr cmyr commented Jan 17, 2025

That is, fonts that live in private repos under the googlefonts org.

That is, fonts that live in private repos under the googlefonts org.

// skip Google Sans, which isn't built with gftools (but other fonts with
// names that begin with 'GoogleSans' might be!)
if ["GoogleSans", "GoogleSans-Italic"].contains(&file_stem.as_ref()) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's file an issue to do this via a config marker rather than hard-coded list.

Why are we looking at the file stem, doesn't the config give us the family name?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the family name isn't mandatory.

The config marker thing is a bit funny, in that the reason we're skipping gftools for these fonts is because they don't have gftools configs; the noto fonts have an extremely similar looking config file but don't build with gftools for legacy reasons, and GoogleSans has a stub I added manually.

basically: does it make sense to have a field in a gftools config file that says "don't build me with gftools"?

LoadRepoError::Io(_)
| LoadRepoError::GitFail(_)
| LoadRepoError::NoCommit { .. }
| LoadRepoError::MissingAuth => SkipReason::GitFail,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If skip reasons surface in the UI or logs I suggest having one specifically for missing auth

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

actually I think SkipReason is now completely unused, can delete in a followup.

@cmyr cmyr added this pull request to the merge queue Jan 17, 2025
Merged via the queue into main with commit 507391f Jan 17, 2025
10 checks passed
@cmyr cmyr deleted the branded-crater branch January 17, 2025 21:45
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