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

adjust the API to Glyphs.app #298

Merged
merged 1 commit into from
Dec 18, 2017
Merged

Conversation

schriftgestalt
Copy link
Collaborator

No description provided.

@anthrotype
Copy link
Member

thanks georg, sorry for getting back to this so late.
you added a property getter for gridLength, but since there isn't a corresponding setter, it is a read-only property and the tests fail with AttributeError.
If this is meant to be writable, could you please add a setter as well?

@schriftgestalt
Copy link
Collaborator Author

The gridLength property is not writeable. Only grid and gridSubDivisions. The gridLenght is just the combination of the other two.

@anthrotype
Copy link
Member

good to know, then the tests need to be changed accordingly

@schriftgestalt
Copy link
Collaborator Author

This is a bit confusing as there was a writeable gridLenght property before ;(

@anthrotype
Copy link
Member

ah, ok so the API change was to make it read-only?

@schriftgestalt
Copy link
Collaborator Author

the value that was accessible in gridLength is now in grid. And then there is a new property that is called (confusingly) gridLength. That change changes the API to match Glyphs.app. There it developed to be like that over the years.

@anthrotype
Copy link
Member

I'm ok with sync-ing with the latest Glyphs.app API, whatever you say that is. But we need to address the test failure before we can merge.

@mashabow
Copy link
Contributor

Thanks Georg!

@anthrotype This is the original thread on the Glyphs forum, FYI: https://forum.glyphsapp.com/t/grid-properties-inconsistency-between-python-api-and-glyphs-file/7779

@anthrotype
Copy link
Member

ok I see now, thanks @mashabow. I'll merge this and then I fix the test (removing the line that attempts to modify gridLength, as it's now read-only).

@anthrotype anthrotype merged commit 28783f7 into googlefonts:master Dec 18, 2017
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