-
-
Notifications
You must be signed in to change notification settings - Fork 7.9k
Remove some uses of unicode_literals #10044
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
Conversation
when the home directory contains non-ASCII characters. This also went ahead and removed unicode_literals from most test modules, opting instead to only use unicode literals explicitly for string literals containing non-ASCII characters (naturally this only impacts Python 2; on Python 3 all strings are unicode literals by default). This has all tests passing still on Python 2/3 *without* non-ASCII home directory.
…y home directory set to a non-ASCII name
Some minor failures on OSX--not surprising since I didn't test there. Should be easy to fix. |
… Python 2 leave str as str instead of converting to unicode).
I am not super enthused about this just due to not having a clear picture of what the fall-out might be. Having a mix of unicode literals or not seems worse than none at all or the many work-arounds, but it would not take much to convince me I am wrong. This will hopefully be moot on master in a few months (after 2.2 master will go python 3 only, probably 3.5+). |
FWIW I believe the fix is "correct" (in the sense that there are many places where Py2 wants a non-unicode string (and Py3 a regular, unicode string) so we ended up having to wrap string literals in str() to convert them back from unicode to non-unicode)). Removing the calls to |
lib/matplotlib/rcsetup.py
Outdated
@@ -409,7 +407,15 @@ def deprecate_axes_colorcycle(value): | |||
validate_colorlist = _listify_validator(validate_color, allow_stringlist=True) | |||
validate_colorlist.__doc__ = 'return a list of colorspecs' | |||
|
|||
validate_stringlist = _listify_validator(six.text_type) | |||
def validate_string(s): | |||
if isinstance(s, str): # Always leave str as str |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
isinstance(s, (str, six.text_type))
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I left the separate cases here since I thought it made the intent clearer, but it could just as easily be combined.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
except for minor stylistic point
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with @anntzer (including his one suggested change); additional notes will go in a regular comment.
I did not think that using |
I'm with you on that and agree caution is good. To give an example though, I tried removing It's not actually such a problem though since it's pretty self-contained.
+1 Though in the meantime I'm still going to be needing Python 2.7 support for a while, unfortunately, so it would be good to backport changes like this. |
PR Summary
This provides a minimal fix to the issue I described in #10017. This does not attempt to go through and remove all instances of
unicode_literals
, which would be rash. Instead it just removes it frommatplotlib.__init__
in order to fix the problem with directories containing non-ASCII characters as I described in the issue. It also removesunicode_literals
from most test modules, opting instead to use unicode literals only for specific strings that contain non-ASCII characters. Finally, it fixes a handful of other modules that were then necessary to update in order for the tests to pass.With these changes the test suite passed for me on Linux (minus some tests that were skipped due to not having the prerequisites, so this is something that would still need to be checked) on Python 2 and 3, and with home directories that both do or do not contain non-ASCII characters.
PR Checklist