Skip to content

Add instances_name_length_req check #5035

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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

khaledhosny
Copy link
Contributor

@khaledhosny khaledhosny commented Jun 29, 2025

Description

Add instances_name_length_req check, similar to name_length_req, but checks the family and subfamily names of the instances, computed from STAT table.

Add the new check to Microsoft profile.

Also add failing test case for the new check as well as name_length_req.

Checklist

  • update CHANGELOG.md
  • wait for the tests to pass
  • request a review

@felipesanches
Copy link
Collaborator

felipesanches commented Jun 30, 2025

This is a new check added only to Microsoft profile.

@khaledhosny, do you think this check might be useful for other vendors or do you think it is certainly vendor-specific?

Option A

If you think it might be useful for other vendors, we'll add it to their "pending_review" lists.

Option B

On the other hand, if you think it is certainly vendor-specific, we'll rename it to microsoft/instances_name_length_req and move its source code to Lib/fontbakery/checks/vendorspecific/microsoft/instances_name_length_req.py.

@khaledhosny
Copy link
Contributor Author

This is a new check added only to Microsoft profile.

@khaledhosny, do you think this check might be useful for other vendors or do you think it is certainly vendor-specific?

It should be the same as name_length_req.

@felipesanches
Copy link
Collaborator

This is a new check added only to Microsoft profile.

@khaledhosny, do you think this check might be useful for other vendors or do you think it is certainly vendor-specific?

Option A

If you think it might be useful for other vendors, we'll add it to their "pending_review" lists.

Option B

On the other hand, if you think it is certainly vendor-specific, we'll rename it to microsoft/instances_name_length_req and move its source code to Lib/fontbakery/checks/vendorspecific/microsoft/instances_name_length_req.py.

@khaledhosny please choose option A or B.

@khaledhosny
Copy link
Contributor Author

I have no opinion here, it is up to you. name_length_req is not included in any other profile, and it does not currently have a vendor prefix (it was named com.microsoft/check/name_length_req when I submitted it). The two checks are similar and should be named similarly, giving one of them a vendor prefix and not the other seems inconsistent.

@felipesanches
Copy link
Collaborator

I have no opinion here, it is up to you. name_length_req is not included in any other profile, and it does not currently have a vendor prefix (it was named com.microsoft/check/name_length_req when I submitted it). The two checks are similar and should be named similarly, giving one of them a vendor prefix and not the other seems inconsistent.

OK. Your answer sounds like option A. So I'll add it to the "pending review" lists of all other profiles, signaling that we believe this should be considered for broader adoption.

@khaledhosny khaledhosny force-pushed the instances_name_length_req branch from 648f455 to 9dfaf77 Compare June 30, 2025 22:46
@khaledhosny
Copy link
Contributor Author

OK. Your answer sounds like option A. So I'll add it to the "pending review" lists of all other profiles, signaling that we believe this should be considered for broader adoption.

I’ve added it to pending review lists of various vendor profiles.

@khaledhosny khaledhosny requested a review from felipesanches July 1, 2025 16:15
@felipesanches felipesanches added this to the 1.1.0 milestone Jul 4, 2025
@felipesanches felipesanches self-assigned this Jul 4, 2025
Similar to name_length_req, but checks the family and subfamily names of
the instances, computed from STAT table.

Add the new check to Microsoft profile.

Also add failing test case for the new check as well as name_length_req.
@felipesanches felipesanches force-pushed the instances_name_length_req branch from 7acf8f9 to dfc4473 Compare July 12, 2025 00:56
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