8000 FIX: always decode bytes as utf-8 in AFM headers by tacaswell · Pull Request #8021 · matplotlib/matplotlib · GitHub
[go: up one dir, main page]
Skip to content

FIX: always decode bytes as utf-8 in AFM headers #8021

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 7 commits into from
Mar 20, 2017
Merged

FIX: always decode bytes as utf-8 in AFM headers #8021

merged 7 commits into from
Mar 20, 2017

Conversation

tacaswell
Copy link
Member

closes #3517

@tacaswell tacaswell added this to the 2.0.1 (next bug fix release) milestone Feb 4, 2017
@tacaswell tacaswell added the Release critical For bugs that make the library unusable (segfaults, incorrect plots, etc) and major regressions. label Feb 5, 2017
@tacaswell
Copy link
Member Author

This is a 'can-not-import-bug', but has been here a long time, the critical flag is to make sure we talk about it.

To test this I think we need to find a debian font which has a non-ascii font family name.

@NelleV
Copy link
Member
NelleV commented Feb 16, 2017

I think just checking that afm._to_str works on non ascii path is enough.

Copy link
Member
NelleV commented Mar 12, 2017

Can I still review this PR? (cause it looks very fine to me 👍 for merging!)

QuLogic
QuLogic reviewed Mar 12, 2017
View reviewed changes
lib/matplotlib/tests/test_afm.py
@@ -0,0 +1,13 @@
import matplotlib.afm as afm
Copy link
Member

Choose a reason for hiding this comment

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

This file is missing our standard __future__ imports.

Copy link
Member

Choose a reason for hiding this comment

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

But the future import is useless for this file.

Copy link
Member Author

Choose a reason for hiding this comment

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

actually, it might be harmful due to the 'unicode_literals'...

Copy link
Member

Choose a reason for hiding this comment

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

In the future, if anyone adds new tests here, then they won't have to worry about adding in these imports if necessary. Easy consistency is important for tests.

Copy link
Member

Choose a reason for hiding this comment

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

This is the kind of practices that break static checkers such as pyflakes with very little benefit considering a forgotten import should break the test.

Copy link
Member

Choose a reason for hiding this comment

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

I've never seen a static checker complain about __future__ imports; how would they even know if you used, e.g., division with types that were integers?

This is about consistent Python 2/3 behaviour, not some random import.

QuLogic
QuLogic approved these changes Mar 16, 2017
View reviewed changes
lib/matplotlib/tests/test_afm.py Outdated
unicode_literals)

import matplotlib.afm as afm
import six
Copy link
Member

Choose a reason for hiding this comment

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

It appears that other changes mean this can be removed?

lib/matplotlib/tests/test_afm.py Outdated

def test_nonascii_str():
# This tests that we also decode bytes as utf-8 properly.
# Else, font files with non ascii caracters fail to load.
Copy link
Member

Choose a reason for hiding this comment

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

caracters -> characters

@NelleV NelleV changed the title FIX: always decode bytes as utf-8 in AFM headers [MRG+1] FIX: always decode bytes as utf-8 in AFM headers Mar 19, 2017
dstansby
dstansby approved these changes Mar 19, 2017
View reviewed changes
Copy link
Member
@dstansby dstansby left a comment

Choose a reason for hiding this comment

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

👍 since @NelleV made some changes I will leave this for @tacaswell to merge if he's happy with them

@dstansby dstansby changed the title [MRG+1] FIX: always decode bytes as utf-8 in AFM headers [MRG+2] FIX: always decode bytes as utf-8 in AFM headers Mar 19, 2017
QuLogic
QuLogic previously requested changes Mar 19, 2017
View reviewed changes
lib/matplotlib/tests/test_afm.py
@@ -0,0 +1,14 @@
from __future__ import (absolute_import, division, print_function,
Copy link
Member

Choose a reason for hiding this comment

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

Needs an encoding comment; this fails on Python 2.

Copy link
Member

Choose a reason for hiding this comment

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

Fixed!

@tacaswell tacaswell dismissed QuLogic’s stale review March 20, 2017 19:09

It now imports on py2

@tacaswell tacaswell merged commit 47eb989 into matplotlib:v2.0.x Mar 20, 2017
@tacaswell tacaswell deleted the fix_nonascii_afm branch March 20, 2017 19:09
@QuLogic QuLogic changed the title [MRG+2] FIX: always decode bytes as utf-8 in AFM headers FIX: always decode bytes as utf-8 in AFM headers Mar 23, 2017
@tacaswell tacaswell mentioned this pull request Sep 17, 2017
Closed
@javadr javadr mentioned this pull request Sep 18, 2017
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@NelleV NelleV NelleV left review comments

@dstansby dstansby dstansby approved these changes

@QuLogic QuLogic QuLogic left review comments

Assignees
No one assigned
Labels
Release critical For bugs that make the library unusable (segfaults, incorrect plots, etc) and major regressions.
Projects
None yet
Milestone
v2.0.1
Development

Successfully merging this pull request may close these issues.

4 participants
@tacaswell @NelleV @QuLogic @dstansby
0