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

Allow GSFont object to be passed to build_masters directly (in place of a path) #1029

Merged
merged 1 commit into from
Sep 4, 2024

Conversation

Hoolean
Copy link
Contributor

@Hoolean Hoolean commented Sep 4, 2024

This helps avoid a costly and redundant save-and-load for Glyphs sources that are manipulated in memory prior to conversion.

This helps avoid a costly and redundant save-and-load for Glyphs sources
that are manipulated in memory prior to conversion.
@anthrotype
Copy link
Member

i'm not necessarily opposed to this, however note that this build_masters function will continue to interact with the filesystem (it writes out the DS and the master UFOs to disk). If you don't want to do that, wouldn't it be better for your code to actually call the inner to_designspace function which this build_masters uses?

@anthrotype
Copy link
Member

i suppose this is still useful if one wants do to do what build_masters does but also wants to do some extra pre-processing on the GSFont before it is turned into DS+UFO, so LGTM

@Hoolean
Copy link
Contributor Author

Hoolean commented Sep 4, 2024

yes, that last comment is our exact use-case 🚀

@Hoolean
Copy link
Contributor Author

Hoolean commented Sep 4, 2024

Ta for speedy review Cosimo :) I don't have merge rights here, so I'm afraid I'll need to ask a maintainer to hit the final merge button for me too

@anthrotype anthrotype merged commit 6b9c4ee into googlefonts:main Sep 4, 2024
8 checks passed
@Hoolean Hoolean deleted the build-loaded branch September 4, 2024 14:43
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