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

Add support for ot-metrics #204

Merged
merged 4 commits into from
Aug 28, 2024
Merged

Add support for ot-metrics #204

merged 4 commits into from
Aug 28, 2024

Conversation

skef
Copy link
Contributor

@skef skef commented Aug 27, 2024

Some test infrastructure I'm fooling with would benefit from this.

Hoping you might have some advice on whether or how I should test the x/y_variation functions, or if I should just delete those ...

@anthrotype
Copy link
Member

thanks!

some advice on whether or how I should test the x/y_variation functions, or if I should just delete those ...

what's different/special about those as far as testing is concerned?

@skef
Copy link
Contributor Author

skef commented Aug 27, 2024

what's different/special about those as far as testing is concerned?

Looking at the source, they seem to scale the returned values based on em_scalef_x or em_scalef_y, which use scaling factors in the font. But I didn't dig too deeply on how to set those factors. @khaledhosny might have a better idea.

@skef
Copy link
Contributor Author

skef commented Aug 28, 2024

After thinking about this I added a mundane test for each of the methods. Given that uharfbuzz is a very thin layer over HarfBuzz, that's probably fine. Even if these methods aren't useful now, perhaps future additions will make them so.

@khaledhosny
Copy link
Collaborator

I’m not familiar with this API, to be honest.

@anthrotype
Copy link
Member

anthrotype commented Aug 28, 2024

fwiw i'd be fine with merging this as is

@skef
Copy link
Contributor Author

skef commented Aug 28, 2024

If it passes review, including verifying that I'm calling each method I added in the tests in a somewhat substantive way, I think it will be fine.

src/uharfbuzz/_harfbuzz.pyx Outdated Show resolved Hide resolved
src/uharfbuzz/_harfbuzz.pyx Outdated Show resolved Hide resolved
src/uharfbuzz/_harfbuzz.pyx Outdated Show resolved Hide resolved
src/uharfbuzz/_harfbuzz.pyx Outdated Show resolved Hide resolved
src/uharfbuzz/_harfbuzz.pyx Outdated Show resolved Hide resolved
src/uharfbuzz/_harfbuzz.pyx Outdated Show resolved Hide resolved
src/uharfbuzz/_harfbuzz.pyx Outdated Show resolved Hide resolved
src/uharfbuzz/_harfbuzz.pyx Outdated Show resolved Hide resolved
src/uharfbuzz/_harfbuzz.pyx Outdated Show resolved Hide resolved
skef and others added 2 commits August 28, 2024 11:58
Co-authored-by: خالد حسني (Khaled Hosny) <khaled@aliftype.com>
Co-authored-by: خالد حسني (Khaled Hosny) <khaled@aliftype.com>
@skef
Copy link
Contributor Author

skef commented Aug 28, 2024

Sorry, don't know where my head was on this one (on the typing that is -- with the variable stuff I was mostly copying from other parts of the file, unsure of how reliable the cython conversion would be).

@khaledhosny khaledhosny merged commit 69b6587 into harfbuzz:main Aug 28, 2024
4 checks passed
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.

3 participants