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

CamelCase family name check #4037

Merged
merged 3 commits into from
Feb 2, 2023
Merged

CamelCase family name check #4037

merged 3 commits into from
Feb 2, 2023

Conversation

yanone
Copy link
Collaborator

@yanone yanone commented Jan 30, 2023

I discovered that our CamelCase family name check isn't firing for a font I'm working on. I have trouble understanding how the conditions are checked in Fontbakery. The check is currently SKIP'ping for the font in question. Even the test is weird, because I don't understand where the metadata comes from in check["font_metadata"] in the test.
I only added a test so far.
Please have a look.

@felipesanches
Copy link
Collaborator

What's the font family you're talking about here?

@felipesanches
Copy link
Collaborator

Is it the KonKhmer_SleokChher.ttf that you added to the PR?

@yanone
Copy link
Collaborator Author

yanone commented Jan 30, 2023

Yes

@yanone
Copy link
Collaborator Author

yanone commented Jan 30, 2023

Later I want to expand the check to FAIL for the underscore as well, but first I want to get it not skipping for CamelCase.

I added another older font with CamelCase to the exceptions list, and now the check is skipping 119 times for the GF library, but those could be all known exceptions.

However, it's skipping the current font, and I'm suspecting either of the two conditions of the check com.google.fonts/check/metadata/fontname_not_camel_cased not returning usable values and thus skipping the check.

@felipesanches
Copy link
Collaborator

Is this the full family name? "Kon Khmer Sleok Chher"? Or is it something else similar to that?

@yanone
Copy link
Collaborator Author

yanone commented Jan 30, 2023

It's "KonKhmer_SleokChher"

@felipesanches
Copy link
Collaborator

Does the family name actually include an underscore?!
Between two camel-cased terms?!
oh boy...

@yanone
Copy link
Collaborator Author

yanone commented Jan 31, 2023

For the test, is there a way to invoke the test exactly like as if it was invoked from the command line, as in just sending the font path and nothing else? This habit of constructing parameters in the test to hand over to the check is very error-prone.

@felipesanches
Copy link
Collaborator

Here, the check runs correctly:

Screenshot from 2023-02-02 02-16-54

Maybe you forgot to provide a METADATA.pb file with the correct filename field? That would cause the check to SKIP indeed.

I forged a fake METADATA.pb file here and that was enough to see the correct check results:
Screenshot from 2023-02-02 02-17-34

@felipesanches felipesanches force-pushed the gf-font-name-compliance branch from f8fb26d to 4e4270f Compare February 2, 2023 14:35
@felipesanches felipesanches marked this pull request as ready for review February 2, 2023 14:35
@felipesanches felipesanches merged commit 9da7e97 into main Feb 2, 2023
@felipesanches felipesanches added this to the 0.8.11 milestone Feb 2, 2023
@yanone
Copy link
Collaborator Author

yanone commented Feb 2, 2023

I wasn't done with this at all which is why I marked this as draft like you asked me to. Only just got started.

We need tests to run on the actual fonts as much as possible, not on additional metadata files that only exist much later in the onboarding process when making PRs to GF. While I author/QA fonts, METADATA.pb doesn't exist yet. In this case, the family name needs to be read from the font file and then checked.

@yanone yanone deleted the gf-font-name-compliance branch February 3, 2023 10:00
@felipesanches
Copy link
Collaborator

felipesanches commented Feb 3, 2023

I agree with the proposal of moving the checking to the values on the binaries, so that problems surface earlier during font development. (and then keeping a much simpler check for later in the pipeline ensuring METADATA.pb matches the value found in the binary)

And I'm glad to see you working on this at issue #4049 / PR #4050

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