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

refactor name checks (attempt 2) #3800

Closed
wants to merge 42 commits into from
Closed

Conversation

m4rc1e
Copy link
Collaborator

@m4rc1e m4rc1e commented Jun 10, 2022

Description

This PR replaces the name checks we currently have by using the name builders found in the axis registry, googlefonts/axisregistry#31. It will also check the fvar instances and stat entries.

I've decided to group the checks together and display the information as tables.

Screenshot 2022-06-10 at 15 13 38

@felipesanches still wip but getting there. My aim is to keep the existing tests so we know we're safe. The name builders also have extensive tests, https://github.com/googlefonts/axisregistry/blob/main/tests/test_names.py

To Do

  • update CHANGELOG.md
  • wait for all checks to pass
  • request a review

@m4rc1e
Copy link
Collaborator Author

m4rc1e commented Jun 10, 2022

It looks rather snazzy as markdown.

Screenshot 2022-06-10 at 15 17 04

obvs I will add the actual names of each nameID.

@felipesanches felipesanches added this to the 0.8.10 milestone Jun 10, 2022
@m4rc1e m4rc1e force-pushed the name-checks2 branch 3 times, most recently from d256c1b to cc28cfc Compare June 23, 2022 17:02
@felipesanches
Copy link
Collaborator

I've just cherry-picked 2107400 into main

Thanks, @m4rc1e

@m4rc1e
Copy link
Collaborator Author

m4rc1e commented Jun 24, 2022

Cool! thanks Felipe. I'm just wrapping up here.

I've decided to parametrize the old tests and keep em in for safety reasons. Hoping to be done by today.

@m4rc1e m4rc1e marked this pull request as ready for review June 27, 2022 15:33
@m4rc1e
Copy link
Collaborator Author

m4rc1e commented Jun 27, 2022

@felipesanches I guess this is ready for a first review. Since I've just parametrised the old name checks, I don't think we need a hangout where I try and convince you to merge this ;-).

felipesanches pushed a commit to felipesanches/fontbakery that referenced this pull request Jun 28, 2022
@felipesanches
Copy link
Collaborator

Thanks, @m4rc1e

I have squashed all your commits into a single one and opened a followup PR at #3820. I'll add my own commits on top of that.

felipesanches pushed a commit to felipesanches/fontbakery that referenced this pull request Jun 28, 2022
felipesanches added a commit to felipesanches/fontbakery that referenced this pull request Jun 28, 2022
felipesanches added a commit to felipesanches/fontbakery that referenced this pull request Jun 28, 2022
m4rc1e added a commit to felipesanches/fontbakery that referenced this pull request Jul 28, 2022
m4rc1e pushed a commit to felipesanches/fontbakery that referenced this pull request Jul 28, 2022
felipesanches pushed a commit to felipesanches/fontbakery that referenced this pull request Aug 2, 2022
felipesanches added a commit to felipesanches/fontbakery that referenced this pull request Aug 2, 2022
felipesanches pushed a commit that referenced this pull request Aug 2, 2022
felipesanches added a commit that referenced this pull request Aug 2, 2022
@yanone yanone mentioned this pull request May 8, 2024
13 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants