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

Py3k: reload->imp.reload #1033

Merged
merged 1 commit into from Jul 21, 2012
Merged

Conversation

bfroehle
Copy link
Contributor

In Python 3, plt.switch_backend() breaks because 2to3 does not correct "reload":

/home/bfroehle/.local/lib/python3.2/site-packages/matplotlib/pyplot.py in switch_backend(newbackend)
    117     global new_figure_manager, draw_if_interactive, _show
    118     matplotlib.use(newbackend, warn=False)
--> 119     reload(matplotlib.backends)
    120     from matplotlib.backends import pylab_setup
    121     new_figure_manager, draw_if_interactive, _show = pylab_setup()
NameError: global name 'reload' is not defined

It'd be relatively easy to work around this with a simple, but somewhat ugly fix:

try: reload
except: from imp import reload

@@ -55,6 +55,11 @@
LinearLocator, LogLocator, AutoLocator, MultipleLocator,\
MaxNLocator

try:
reload
except:
Copy link
Member

Choose a reason for hiding this comment

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

pedantic: don't do bare except clauses.

Is there any risk of 2to3 getting patched for this and causing problems?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed the bare except clause.

No, I'd expect 2to3 to apply the fix like they do for intern, at which point the try/except block just becomes irrelevant.

--- tmp.py  (original)
+++ tmp.py  (refactored)
@@ -1,6 +1,7 @@
+import sys
 try:
     intern
 except NameError:
     from sys import intern

-intern("Hello")
+sys.intern("Hello")

@pelson
Copy link
Member

pelson commented Jul 21, 2012

Maybe a comment as to why the whole try...except block exists wouldn't go amiss. (And then the meaningless comment "Python 3" should be removed)

@WeatherGod
Copy link
Member

Actually, I think just having the "# Python 3" comment is sufficient since it indicates that that is how one should import this module in py3k. I am going to go ahead and merge this. Thanks for the patch!

WeatherGod added a commit that referenced this pull request Jul 21, 2012
@WeatherGod WeatherGod merged commit 58581c7 into matplotlib:master Jul 21, 2012
@pelson
Copy link
Member

pelson commented Jul 22, 2012

Actually, I think just having the "# Python 3" comment is sufficient since it indicates that that is how one should import this module in py3k.

I disagree. Stylistically it is unelegant (IMHO) to put a no-op (i.e. see if a builtin is available) inside a try statement. I would have preferred (untested) something like:

try:
   from imp import reload
except ImportError:
   pass # with python < 2.7 reload is a builtin 

I'm not going to ask that this gets resolved (because it makes no functional difference and its only 2 lines), but I would like to make my reasoning for raising a review action clear.

@bfroehle: Thanks for contributing, it is really valuable to us that as many people as possible can get stuck into the matplotlib code and make improvements.

@bfroehle
Copy link
Contributor Author

@pelson: Thanks for the review. I actually looked through the matplotlib source to see if there was a preferred way to accomplish this. The proposed patch here was quite common, but also common was a construction which you might have found to be preferable:

if sys.version_info[0] >= 3:
    from imp import reload

The actual patch itself here was styled after some other snippets from the source:

try:
    raw_input
except NameError:
    # Python 3
    raw_input = input
try:
    unichr
except NameError:
    # Python 3
    unichr = chr

And there are plenty other "no-op" try statements scattered throughout the source, including:

try:
    set
except NameError:
    from sets import Set as set
try:
    np.copyto
except AttributeError:
    _putmask = np.putmask
else:
    def _putmask(a, mask, values):
        return np.copyto(a, values, where=mask)

@pelson
Copy link
Member

pelson commented Jul 22, 2012

@bfroehle: Thanks for looking through the codebase to identify implemented patterns, I found that combined with your possible alternatives really interesting an illuminating. As you guessed, my preference is the explicit version check for is readability, but you are right to point out pre-existing code as a precedent. Thanks again for this PR.

@takluyver
Copy link
Contributor

This didn't get moved across with commit 865f1c0. I'll see if I can do a quick PR.

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