Cleanup unused variables and imports #11994

timhoffm · 2018-09-01T21:57:40Z

PR Summary

Continuation of #11958

This fixes a number of lgtm alerts:

unused imports and other import cleanups
unused variables

anntzer · 2018-09-01T22:00:40Z

lib/matplotlib/__init__.py

 import inspect
-from inspect import Parameter


Leave this import? (I tend to import classes (and nothing else) by unqualified name, as a rule.)

Do we have a policy on this? It seems a bit imbalanced that we do

import inspect from inspect import Parameter inspect.signature() Parameter

Generally, I would recommend that we do not import individual classes if they are only rarely used. Importing classes and functions pollutes the namespace more. Also, when it's just rarely used, I usually wouldn't know what Parameter is, whereas inspect.Parameter is quite clear.

https://www.python.org/dev/peps/pep-0008/#imports

When importing a class from a class-containing module, it's usually okay to spell this:

from myclass import MyClass from foo.bar.yourclass import YourClass

If this spelling causes local name clashes, then spell them explicitly:

import myclass import foo.bar.yourclass

and use "myclass.MyClass" and "foo.bar.yourclass.YourClass".

https://docs.python-guide.org/writing/structure/#modules

Using from modu import func [...] only advantage over a simpler import modu is that it will save a little typing. [...]
Being able to tell immediately where a class or function comes from, as in the modu.func idiom, greatly improves code readability and understandability in all but the simplest single file projects.

Parameter is just used in one context (4 occurences there). To me explicitness outweighs brevity here.

anntzer · 2018-09-01T22:01:15Z

lib/matplotlib/cbook/__init__.py

@@ -29,7 +26,6 @@
 import types
 import warnings
 import weakref
-from weakref import WeakMethod


same as above

Just used once: IMO explicitness > brevity

anntzer · 2018-09-01T22:02:52Z

lib/matplotlib/cbook/__init__.py

    pid = os.getpid()
    if sys.platform == 'sunos5':
        try:
-            a2 = Popen(['ps', '-p', '%d' % pid, '-o', 'osz'],
-                       stdout=PIPE).stdout.readlines()
+            a2 = subprocess.Popen(


that should use check_output...
(or more precisely I think a utility function such as report_memory should probably not support sunos5, unless someone specifically asks for it...)

Didn't want to mix the rather technical changes in the PR with other code changes, to kkep the review simpler. But since you asked for it, here it is.

anntzer · 2018-09-01T22:05:13Z

lib/matplotlib/cbook/__init__.py

@@ -2009,7 +2007,7 @@ def _warn_external(message, category=None):
    etc.).
    """
    frame = sys._getframe()
-    for stacklevel in itertools.count(1):
+    for stacklevel in itertools.count(1):  # lgtm[py/unused-loop-variable]


really, lgtm can't see that stacklevel is used below the loop?...

Not using the loop variable within the loop is often an error, even if it's used afterwards. I think this is what they want to indicate here.

Frankly, I don't like peppering the code with comments to appease bad linters.

Well, I think that the alert is a valid concern, and a statement that stacklevel is intentionally not used in the loop is justified here. I did not put comments in other places where the linting was just not good enough.

I guess I should just write something like (untested)

def recur(f, x0): return itertools.accumulate(lambda x, _: f(x), itertools.repeat(x0)) stacklevel = 1 + next( i for i, fr in enumerate(recur(lambda fr: fr.f_back, sys._getframe())) if not re.match(r"\A(matplotlib|mpl_toolkits)(\Z|\.)", fr.f_globals["name"])

and keep lgtm happy :p
(abusing accumulate to repeatedly just apply a function is actually documented as an example in https://docs.python.org/3/library/itertools.html#itertools.accumulate...)

anntzer · 2018-09-01T22:05:59Z

lib/matplotlib/figure.py

@@ -23,16 +23,9 @@
 from matplotlib import get_backend

 import matplotlib.artist as martist
-from matplotlib.artist import Artist, allow_rasterization


Same as above re: importing classes.

anntzer · 2018-09-01T22:06:48Z

lib/matplotlib/font_manager.py

@@ -1293,7 +1293,6 @@ def get_font(filename, hinting_factor=None):

    def fc_match(pattern, fontext):
        fontexts = get_fontext_synonyms(fontext)
-        ext = "." + fontext


Or just merge #10933...

anntzer · 2018-09-01T22:07:50Z

lib/matplotlib/offsetbox.py

@@ -33,7 +33,6 @@
 from matplotlib.image import BboxImage

 from matplotlib.patches import bbox_artist as mbbox_artist
-from matplotlib.text import _AnnotationBase


yada yada per above

anntzer · 2018-09-01T22:09:05Z

setupext.py

 import glob
 import hashlib
 import importlib
 import multiprocessing
 import os
-import pathlib
+from pathlib import Path


That doesn't work: there's a class Path later... (even if it does that'd be quite confusing)

The one place, where I thought it was ok to import a class directly because it's in the standard library...

anntzer · 2018-09-01T22:09:32Z

setupext.py

@@ -1012,8 +1011,6 @@ def add_flags(self, ext):
            ext.define_macros.append(('FREETYPE_BUILD_TYPE', 'system'))

    def do_custom_build(self):
-        from pathlib import Path


just replace Path by pathlib.Path here

anntzer · 2018-09-01T22:09:44Z

tools/make_icons.py

@@ -19,11 +19,10 @@

 import numpy as np

-from matplotlib import pyplot as plt
-from matplotlib.font_manager import FontProperties


same as above

QuLogic

I'm okay with everything here except the the LGTM comment that @anntzer pointed out. Since you can't run LGTM locally (right?), it seems superfluous.

QuLogic · 2018-09-06T05:47:13Z

lib/matplotlib/__init__.py

@@ -119,10 +119,8 @@
 import contextlib
 import distutils.version
 import functools
-import io
-import importlib


This should stay here to be sorted.

timhoffm · 2018-09-06T20:19:28Z

For now, I've reverted the changes to class imports, so that this can get merged and #12032 can follow up.

Nevertheless, I think we should adopt a consistent import policy for classes. We currently have a mixture e.g. class AnnotationBbox(martist.Artist, _AnnotationBase):. I will address this in a separate issue.

anntzer · 2018-09-06T21:01:20Z

lib/matplotlib/__init__.py

@@ -1612,7 +1611,8 @@ def param(func):
                _has_varkwargs = True
            else:
                _arg_names.append(p.name)
-        data_param = Parameter('data', Parameter.KEYWORD_ONLY, default=None)
+        data_param = Parameter(


leave as was

anntzer · 2018-09-06T21:06:32Z

lib/matplotlib/offsetbox.py

@@ -35,7 +35,6 @@
 from matplotlib.patches import bbox_artist as mbbox_artist
 from matplotlib.text import _AnnotationBase

-


not to start an endless discussion about whitespace but I'd put two blank lines here :p

NelleV · 2018-09-14T15:47:41Z

Hey @timhoffm . Can you rebase and ping me once rebased?
Thanks,

timhoffm · 2018-09-15T11:04:13Z

Ping @NelleV

NelleV · 2018-09-15T11:28:21Z

I really wish there was a way to tell github "hey, merge this if all CI pass".

NelleV · 2018-09-15T11:44:54Z

Thanks @timhoffm

anntzer reviewed Sep 1, 2018

View reviewed changes

timhoffm force-pushed the lgtm-unused-part2 branch 3 times, most recently from ac728aa to 814768f Compare September 1, 2018 23:46

timhoffm added this to the v3.1 milestone Sep 2, 2018

timhoffm mentioned this pull request Sep 6, 2018

Remove unused imports #12032

Merged

2 tasks

QuLogic reviewed Sep 6, 2018

View reviewed changes

timhoffm force-pushed the lgtm-unused-part2 branch 3 times, most recently from e26e471 to ed06162 Compare September 6, 2018 20:15

anntzer reviewed Sep 6, 2018

View reviewed changes

timhoffm force-pushed the lgtm-unused-part2 branch 2 times, most recently from 6a4e7d9 to 5f6a510 Compare September 6, 2018 21:12

phobson approved these changes Sep 8, 2018

View reviewed changes

jklymak added the status: needs rebase label Sep 14, 2018

Cleanup unused variables and imports

485e0f8

timhoffm force-pushed the lgtm-unused-part2 branch from 5f6a510 to 485e0f8 Compare September 15, 2018 11:03

NelleV merged commit 01d65f3 into matplotlib:master Sep 15, 2018

QuLogic removed the status: needs rebase label Sep 15, 2018

timhoffm deleted the lgtm-unused-part2 branch September 15, 2018 17:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cleanup unused variables and imports #11994

Cleanup unused variables and imports #11994

timhoffm commented Sep 1, 2018

anntzer Sep 1, 2018

timhoffm Sep 1, 2018

anntzer Sep 1, 2018

timhoffm Sep 1, 2018

anntzer Sep 1, 2018

timhoffm Sep 1, 2018

anntzer Sep 1, 2018

timhoffm Sep 1, 2018

anntzer Sep 1, 2018

timhoffm Sep 1, 2018

anntzer Sep 1, 2018

timhoffm Sep 1, 2018

anntzer Sep 7, 2018

anntzer Sep 1, 2018

anntzer Sep 1, 2018

anntzer Sep 1, 2018

anntzer Sep 1, 2018

timhoffm Sep 1, 2018

anntzer Sep 1, 2018

anntzer Sep 1, 2018

timhoffm Sep 1, 2018

anntzer Sep 1, 2018

QuLogic left a comment

QuLogic Sep 6, 2018

timhoffm commented Sep 6, 2018

anntzer Sep 6, 2018

anntzer Sep 6, 2018

NelleV commented Sep 14, 2018

timhoffm commented Sep 15, 2018

NelleV commented Sep 15, 2018

NelleV commented Sep 15, 2018

		@@ -35,7 +35,6 @@
		from matplotlib.patches import bbox_artist as mbbox_artist
		from matplotlib.text import _AnnotationBase

Cleanup unused variables and imports #11994

Cleanup unused variables and imports #11994

Conversation

timhoffm commented Sep 1, 2018

PR Summary

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

QuLogic left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timhoffm commented Sep 6, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NelleV commented Sep 14, 2018

timhoffm commented Sep 15, 2018

NelleV commented Sep 15, 2018

NelleV commented Sep 15, 2018