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

refactor ftface_props example #3630

Merged
merged 3 commits into from Oct 20, 2014
Merged

refactor ftface_props example #3630

merged 3 commits into from Oct 20, 2014

Conversation

twmr
Copy link
Contributor

@twmr twmr commented Oct 10, 2014

BTW this fixes many pep8 errors in this file

@tacaswell
Copy link
Member

Not a huge fan of this change, I think it hides the bit-shift logic under a bit too much clever.

@tacaswell tacaswell added this to the unassigned milestone Oct 10, 2014
@tacaswell tacaswell modified the milestones: v1.5.x, unassigned Oct 10, 2014
@mdboom
Copy link
Member

mdboom commented Oct 10, 2014

Haven't looked at this file in a while. Note that these constants are now defined in the ft2font extension itself, so there's no need to recreate their values here. i.e., one can just do ft2font.SCALABLE.

@twmr
Copy link
Contributor Author

twmr commented Oct 10, 2014

@mdboom just found a typo in the values of the style identifiers in ft2font.cpp

What do you think about my last commit. Is it still too clever?

@tacaswell
Copy link
Member

@thisch This needs a re-base

@mdboom Can you comment on the ft2font typo?

Signed-off-by: Thomas Hisch <t.hisch@gmail.com>
Signed-off-by: Thomas Hisch <t.hisch@gmail.com>
Signed-off-by: Thomas Hisch <t.hisch@gmail.com>
@twmr
Copy link
Contributor Author

twmr commented Oct 19, 2014

rebased

@mdboom
Copy link
Member

mdboom commented Oct 20, 2014

Thanks. The typo fixed here is legit. Thanks for catching.

I think the latest version of the example file here is much better, and not "too clever". 👍 from me.

mdboom added a commit that referenced this pull request Oct 20, 2014
refactor ftface_props example
@mdboom mdboom merged commit 0a96efb into matplotlib:master Oct 20, 2014
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