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

Use otData for dict to COLR conversion #2181

Merged
merged 5 commits into from
Feb 15, 2021
Merged

Conversation

rsheeter
Copy link
Collaborator

@rsheeter rsheeter commented Feb 9, 2021

Writing per-format functions is tiresome, try to avoid it by leaning on otData. Setting up as PR because I think it's far enough along it might be convenient to be able to comment.

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.

there's a few issues, I left comments inline. I have to say this does increment the level of complexity (potential hidden bugs, difficulty of maintaining, etc.) a little bit. Hopefully that's a good trade-off and I will just get used to that.

Lib/fontTools/colorLib/builder.py Outdated Show resolved Hide resolved
Lib/fontTools/colorLib/builder.py Outdated Show resolved Hide resolved
Lib/fontTools/colorLib/builder.py Outdated Show resolved Hide resolved
Lib/fontTools/colorLib/builder.py Outdated Show resolved Hide resolved
Lib/fontTools/colorLib/builder.py Outdated Show resolved Hide resolved
Lib/fontTools/colorLib/table_builder.py Show resolved Hide resolved
Lib/fontTools/colorLib/table_builder.py Show resolved Hide resolved
Lib/fontTools/colorLib/table_builder.py Show resolved Hide resolved
Lib/fontTools/colorLib/table_builder.py Show resolved Hide resolved
Lib/fontTools/colorLib/table_builder.py Outdated Show resolved Hide resolved
@rsheeter
Copy link
Collaborator Author

I've somehow lost your comment about why meddle with the optional version. Note that I put that back to how it was before.

@rsheeter rsheeter force-pushed the colr_to_from_dicts branch 2 times, most recently from d3cc857 to 385bb5a Compare February 10, 2021 07:29
Lib/fontTools/colorLib/table_builder.py Outdated Show resolved Hide resolved
Lib/fontTools/colorLib/table_builder.py Outdated Show resolved Hide resolved
Tests/colorLib/builder_test.py Outdated Show resolved Hide resolved
Tests/colorLib/builder_test.py Outdated Show resolved Hide resolved
Lib/fontTools/colorLib/builder.py Show resolved Hide resolved
Lib/fontTools/colorLib/builder.py Show resolved Hide resolved
Lib/fontTools/colorLib/builder.py Outdated Show resolved Hide resolved
Lib/fontTools/colorLib/builder.py Outdated Show resolved Hide resolved
@rsheeter rsheeter force-pushed the colr_to_from_dicts branch 3 times, most recently from c929f4b to 6538947 Compare February 10, 2021 17:22
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

@rsheeter
Copy link
Collaborator Author

Made a last few tweaks, ptal, hopefully safe to merge now.

@anthrotype anthrotype merged commit 6106bf7 into master Feb 15, 2021
@anthrotype anthrotype deleted the colr_to_from_dicts branch February 15, 2021 10:36
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

2 participants