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

Remove some uses of unicode_literals #10044

Merged
merged 4 commits into from Dec 21, 2017

Conversation

embray
Copy link
Contributor

@embray embray commented Dec 19, 2017

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 from matplotlib.__init__ in order to fix the problem with directories containing non-ASCII characters as I described in the issue. It also removes unicode_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

  • Has Pytest style unit tests
  • Code is PEP 8 compliant
  • New features are documented, with examples if plot related
  • Documentation is sphinx and numpydoc compliant
  • Added an entry to doc/users/next_whats_new/ if major new feature (follow instructions in README.rst there)
  • Documented in doc/api/api_changes.rst if API changed in a backward-incompatible way

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.
@embray
Copy link
Contributor Author

embray commented Dec 19, 2017

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).
@tacaswell tacaswell added this to the v2.2 milestone Dec 19, 2017
@tacaswell
Copy link
Member

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+).

@anntzer
Copy link
Contributor

anntzer commented Dec 19, 2017

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 str() is also going to happen in a cleanup step after dropping Py2, so it may be just as good to do it now...

@@ -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
Copy link
Contributor

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))

Copy link
Contributor Author

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.

Copy link
Contributor

@anntzer anntzer left a 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

Copy link
Member

@efiring efiring left a 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.

@efiring
Copy link
Member

efiring commented Dec 21, 2017

I did not think that using unicode_literals was a good idea when it was initially adopted by mpl, and I have never used it in moving my own projects to Py3, so I am very sympathetic to this PR. I agree with @tacaswell also, though, in wondering whether it might break user code, even though it looks logical and benign.

@embray
Copy link
Contributor Author

embray commented Dec 21, 2017

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.

I'm with you on that and agree caution is good. To give an example though, I tried removing unicode_literals from all modules, but there are a few that have been very tightly written around the assumption. For example, reworking the backend_ps module to not use it turned out to be a lot of work (I actually have rewritten most of it, but decided it was too big a chance to include in this one).

It's not actually such a problem though since it's pretty self-contained.

This will hopefully be moot on master in a few months (after 2.2 master will go python 3 only, probably 3.5+).

+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.

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

5 participants