search

rougier / freetype-py

Python binding for the freetype library
Other
304 stars 88 forks source link

Improve Variable Font support #118

Closed josh-hadley closed 4 years ago

josh-hadley commented 4 years ago

Updates to several freetype-py modules to improve support for Variable Fonts. Includes tests for added code, as well as an example demonstrating variable font usage.

AppVeyorBot commented 4 years ago

:white_check_mark: Build freetype-py 1.0.130 completed (commit https://github.com/rougier/freetype-py/commit/107b5805dc by @josh-hadley)

rougier commented 4 years ago

Really nice ! Thanks for the PR. This looks good to me. Let's wait a day or two for potential comments and then we merge (maybe you'll need to remind me).

HinTak commented 4 years ago

FWIW, "from ctype import *" and "import ctypes" do different things. In the latter, you still need to do "ctypes.x" in code, vs bare "x" in the former.

josh-hadley commented 4 years ago

FWIW, "from ctype import *" and "import ctypes" do different things. In the latter, you still need to do "ctypes.x" in code, vs bare "x" in the former.

Right. My preference would be from ctypes import (<sequence of each individual thing used from ctypes>), so that only the import statement(s) would change, rather than each line of code where a ctypes item is used. We can discuss more in a later PR 😄

AppVeyorBot commented 4 years ago

:x: Build freetype-py 1.0.131 failed (commit https://github.com/rougier/freetype-py/commit/92f333df78 by @josh-hadley)

AppVeyorBot commented 4 years ago

:white_check_mark: Build freetype-py 1.0.132 completed (commit https://github.com/rougier/freetype-py/commit/bbd2302a0f by @josh-hadley)

rougier commented 4 years ago

@HinTak Yes but I (personally) prefer to prefix, maybe because I'm using numpy a lot and, from time to time, I also need the math module with a lot of overlap/conflicts. Anyway, I can live with the import * if everyone's ok.

HinTak commented 4 years ago

I like what @josh-hadley says, an explicit "from ctypes import x, y, z" ... as long as the list does not get too long. But no need to change what's currently working...

Not a big fan of numpy - I think if you name something the same as a more common package's, you better be 100% compatible...

madig commented 4 years ago

For what it's worth, there is https://pypi.org/project/autoflake/ and maybe https://github.com/quentinsf/dewildcard that can automatically expand star imports.

AppVeyorBot commented 4 years ago

:x: Build freetype-py 1.0.133 failed (commit https://github.com/rougier/freetype-py/commit/83d0c53255 by @josh-hadley)

madig commented 4 years ago

Woops! You might want to run pylint --disable=all --enable=E Lib to find more typos and stuff.

josh-hadley commented 4 years ago

Woops! You might want to run pylint --disable=all --enable=E Lib to find more typos and stuff.

Yeah, I have been using flake8, but had disabled a bunch of warnings to keep the noise down from issues in the existing code...this would've been caught by flake8. It would be good to have pylint or flake8 or similar running in CI to catch these on PR submit :smile:

Related: the appveyor CI failure does not appear to be caused by the most recent commit (https://github.com/rougier/freetype-py/commit/83d0c53255). Seems like a timeout attempting to retrieve build components.

madig commented 4 years ago

Hrm, yeah, we download FreeType and HarfBuzz release tarballs so we can build a static Python wheel. Not sure what's going on there. Needs to be updated anyway, in a separate PR...

Agree on a clean-up PR with linting and stuff.

AppVeyorBot commented 4 years ago

:white_check_mark: Build freetype-py 1.0.135 completed (commit https://github.com/rougier/freetype-py/commit/83d0c53255 by @josh-hadley)

rougier commented 4 years ago

Merged ! Thanks again for the nice PR.