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

Load glyphspackage files #803

Merged
merged 6 commits into from
Aug 31, 2022
Merged

Load glyphspackage files #803

merged 6 commits into from
Aug 31, 2022

Conversation

simoncozens
Copy link
Collaborator

This implements loading .glyphspackages. We can do saving later if we really need to, but for the 80% case (loading fonts for compilations) this gives you useful functionality.

glyphsLib.load now takes a filename or a file object, and if the filename is a directory, the glyphspackage loader is used.

data = openstep_plist.load(
open(infofile, "r", encoding="utf-8"), use_numbers=True
)
data = openstep_plist.load(open(infofile, "r", encoding="utf-8"), use_numbers=True)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it is time we drop these formatting checks. The point is to reduce the noise of pure formatting changes, not make them common.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or we add a GItHub action which fixes up PRs…

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the author of the PR can run the checks before pushing, or they can fix up and then squash the relevant commit before merging to avoid the git noise. I think it's good that we continue to enforce this.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In theory, but that is not what is happening in practice:

$ git log --oneline | grep -i black
ee5e1fbb black
66ce3be8 Black source again
09b68d69 Set black's target version to py37
faccf6f0 Black it out
96e8c060 Make flake8/black happier
ed01e31c Black
54c396db black
2e2cb067 Black/flake8 issues
9e1eee91 Whitespace black happiness
99b812c1 Black
dfb9f2bb Flake8/black
3044a6cf black/flake8. Really I should add a precommit hook.
dbd5c07b black
8d9b8671 manual black
09ad4054 Blackened
baadfaa8 Black-formatted
f735baa2 Black formatted, no other changes
c804b5e0 blacken source to address linting fail
0be52cc7 Format with Black
60fe5656 Update pre-commit hooks, re-blacken
d3733593 Merge pull request #414 from googlei18n/blacken-code
0f5a4ca6 tox: add flake8 config compatible with black
ab2cb42e run black autoformatter on the whole code base
359583d5 Add pre-commit hooks for black, black-docs and pyupgrade to to

$ git log --oneline | grep -i lint
a81d2be2 Fix lints
c32d9141 Fix lints
fc3bbc37 Linting
fe93e69f Lint fix
5082f6cf flake8 lint: refactor chained exception to use `from`
fc32c3c9 Fix lint niggle
ab94e9f9 tox lint happiness
f38d4d63 Lint things
a53e6e49 deploy should wait on both lint and test jobs to complete
6b4357e5 re-add separate lint job running on ubuntu only
4d2d5ddc remove Travis config; run lint on latest 3.x
de306414 Add lint job
9468b1e7 Make the linter happy
6df76cf4 fix lint issue
96114d03 lint fixes
8c224996 lint fixes
1ad88c1d Fix lint and ignore flake's E741
c804b5e0 blacken source to address linting fail
f7c8d3c0 remove trailing whitespace to address linter fails on new tests
e4b5bf6d Update pre-commit deps, fix lints
44af0f8e Merge pull request #417 from madig/minor-lint-fixes
8cd6ec44 travis: add separate Travis 3.6 build for linting/reformatting checks
e23ac7fa Add writer test for GSAnchor, fix some linter warnings

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't like that either, but if it's the small price to pay for consistent style..

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's probably mainly just me being lazy and waiting for the CI to do the tests for me.

Copy link
Member

@anthrotype anthrotype left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks

return data


def load(file):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: "path" can be understood as either a file name or a directory name, whereas "file" is usually just a single file name or the file object

Suggested change
def load(file):
def load(file_or_path):

@anthrotype
Copy link
Member

For reference, this fixes #801

@simoncozens simoncozens merged commit 7aac93d into main Aug 31, 2022
@florianpircher
Copy link

Thank you all ❤️

@simoncozens
Copy link
Collaborator Author

This will need a fontmake PR before it starts magically working. I'll do that soon, up to my elbows in glyphsLib gunk at the moment.

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.

None yet

4 participants