Py3k: reload->imp.reload #1033

Merged
merged 1 commit into from Jul 21, 2012

Projects

None yet

4 participants

@bfroehle
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
@bfroehle bfroehle referenced this pull request in ipython/ipython Jul 21, 2012
Merged

Pylab switch #2179

@WeatherGod WeatherGod and 1 other commented on an outdated diff Jul 21, 2012
lib/matplotlib/pyplot.py
@@ -55,6 +55,11 @@
LinearLocator, LogLocator, AutoLocator, MultipleLocator,\
MaxNLocator
+try:
+ reload
+except:
@WeatherGod
WeatherGod Jul 21, 2012 Member

pedantic: don't do bare except clauses.

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

@bfroehle
bfroehle Jul 21, 2012 Contributor

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
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
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 WeatherGod merged commit 58581c7 into matplotlib:master Jul 21, 2012
@pelson
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
Contributor

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