Code quality fixes for grass package (lib/python) (#576)

This PR fixes Flake8 errors/warnings:

* Do not assign a lambda expression, use a def (E731)
* 'raise NotImplemented' should be 'raise NotImplementedError' (F901)
* Redefinition of unused...from line... (F811)
* Do not compare types, use 'isinstance()' (E721)
* Module level import not at top of file (E402)

And additionally it unifies checks for Python 3 using sys.version_info.major.

I also moved serious errors to be resolved up in the Flake8 configuration file and whitespace errors down. Details on some of the fixes follow.

Module level import: Fixing by actually moving imports (hashlib) and moving the non-importing code down after the imports (matplotlib.use and Python 2/3 variables). Several files ignored using the per-file-ignores. These may or may not be substantiated as exceptions, so keeping them in the config file rather than in the file itself to have here a clear warning that the check was disabled. However, in general, E402 should be fixable by moving code around and couple inline ignores like in the case of matplotlib.use in gunittest/multireport.

sys.version_info.major: Now all checks for Python 3 which are using sys.version_info.major are using >= operator because we are actually interested in 'not Python 2' and we assume Python 4 will behave more like Python 3 and not like Python 2.

Redefinition of unused...from line... seems to mostly report things imported twice in our case.

NotImplemented versus NotImplementedError:  NotImplemented is a constant for implementing binary special methods. NotImplementedError is an exception indicating real implementation still needs to be added.

Lambdas versus defs: Lambdas are for embedding into larger expressions. When assigned,
it is just a strange syntax for a function definition because def can appear anywhere in the code already.

lib/python/.flake8

@@ -1,24 +1,19 @@
 [flake8]
 ignore =
-    E262, # inline comment should start with '# '
-    E265, # block comment should start with '# '
-    E266, # too many leading '#' for block comment
-    E402, # module level import not at top of file
-    E502, # the backslash is redundant between brackets
     E711, # comparison to None should be 'if cond is None:'
     E712, # comparison to True should be 'if cond is True:' or 'if cond:'
-    E721, # do not compare types, use 'isinstance()'
     E722, # do not use bare 'except'
-    E731, # do not assign a lambda expression, use a def
     E741, # ambiguous variable name 'l'
     F401, # '.reader.BandReferenceReader' imported but unused
     F403, # 'from ctypes import *' used; unable to detect undefined names
     F405, # 'RasterRow' may be undefined, or defined from star imports: ctypes, grass.pygrass.raster, grass.pygrass.vector
-    F811, # redefinition of unused 'utils' from line 26
-    F821, # undefined name '_'
     F841, # local variable 't0' is assigned to but never used
-    F901, # 'raise NotImplemented' should be 'raise NotImplementedError'
     W605, # invalid escape sequence '\_'
+    E262, # inline comment should start with '# '
+    E265, # block comment should start with '# '
+    E266, # too many leading '#' for block comment
+    F821, # undefined name '_'
+    E502, # the backslash is redundant between brackets
     W291, # trailing whitespace
     W292, # no newline at end of file
     W293, # blank line contains whitespace
@@ -57,6 +52,19 @@ ignore =
     E305, # expected 2 blank lines after class or function definition, found 1
     E401, # multiple imports on one line
 
+per-file-ignores =
+    # C wrappers call libgis.G_gisinit before importing other modules.
+    # TODO: Is this really needed?
+    pygrass/vector/__init__.py: E402,
+    pygrass/raster/__init__.py: E402,
+    pygrass/utils.py: E402,
+    # Current benchmarks/tests are changing sys.path before import.
+    # Possibly, a different approach should be taken there anyway.
+    pygrass/tests/benchmark.py: E402,
+    # Configuration file for Sphinx:
+    # Ignoring import/code mix and line length.
+    docs/conf.py: E402, E501
+
 max-line-length = 88
 exclude =
     .git,

lib/python/ctypes/ctypesgencore/parser/cgrammar.py

@@ -29,7 +29,7 @@ from . import preprocessor
 from . import yacc
 
 
-if sys.version_info.major == 3:
+if sys.version_info.major >= 3:
     long = int
 
 

lib/python/ctypes/ctypesgencore/parser/pplexer.py

@@ -24,7 +24,7 @@ from .lex import TOKEN
 
 
 PY2 = True
-if sys.version_info.major == 3:
+if sys.version_info.major >= 3:
     PY2 = False
     long = int
 

lib/python/gunittest/checkers.py

@@ -13,6 +13,7 @@ import os
 import sys
 import re
 import doctest
+import hashlib
 
 from grass.script.utils import decode, encode, _get_encoding
 
@@ -555,8 +556,6 @@ def check_text_ellipsis_doctest(reference, actual):
                                 optionflags=doctest.ELLIPSIS)
 
 
-import hashlib
-
 # optimal size depends on file system and maybe on hasher.block_size
 _BUFFER_SIZE = 2**16
 

lib/python/gunittest/multireport.py

@@ -18,17 +18,17 @@ import datetime
 import operator
 from collections import defaultdict, namedtuple
 
-
-# TODO: we should be able to work without matplotlib
-import matplotlib
-matplotlib.use('Agg')
-import matplotlib.pyplot as plt
-from matplotlib.dates import date2num
-
 from grass.gunittest.checkers import text_to_keyvalue
 from grass.gunittest.utils import ensure_dir
 from grass.gunittest.reporters import success_to_html_percent
 
+# TODO: we should be able to work without matplotlib
+import matplotlib
+matplotlib.use('Agg')
+# This counts as code already, so silence "import not at top of file".
+# Perhaps in the future, switch_backend() could be used.
+import matplotlib.pyplot as plt  # noqa: E402
+from matplotlib.dates import date2num  # noqa: E402
 
 class TestResultSummary(object):
     def __init__(self):

lib/python/imaging/images2swf.py

@@ -425,7 +425,7 @@ class Tag:
 
     def ProcessTag(self):
         """ Implement this to create the tag. """
-        raise NotImplemented()
+        raise NotImplementedError()
 
     def GetTag(self):
         """ Calls processTag and attaches the header. """

lib/python/pygrass/modules/grid/grid.py

@@ -169,12 +169,15 @@ def copy_groups(groups, gisrc_src, gisrc_dst, region=None):
     :returns: None
 
     """
+
+    def rmloc(r):
+        return r.split('@')[0] if '@' in r else r
+
     env = os.environ.copy()
     # instantiate modules
     get_grp = Module('i.group', flags='lg', stdout_=sub.PIPE, run_=False)
     set_grp = Module('i.group')
     get_grp.run_ = True
-    rmloc = lambda r: r.split('@')[0] if '@' in r else r
 
     src = read_gisrc(gisrc_src)
     dst = read_gisrc(gisrc_dst)

lib/python/pygrass/raster/abstract.py

@@ -527,7 +527,7 @@ class RasterAbstractBase(object):
         :param point: pair of coordinates in tuple object or class object with coords() method
         """
         # Check for tuple
-        if type(point) != type([]) and type(point) != type(()):
+        if not isinstance(point, list) and not isinstance(point, tuple):
             point = point.coords()
 
         if not region:

lib/python/pygrass/rpc/__init__.py

@@ -23,7 +23,7 @@ from grass.pygrass.raster import *
 import grass.lib.gis as libgis
 from .base import RPCServerBase
 from grass.pygrass.gis.region import Region
-import grass.pygrass.utils as utils
+from grass.pygrass import utils
 import logging
 
 ###############################################################################
@@ -434,7 +434,6 @@ class DataProvider(RPCServerBase):
 
 if __name__ == "__main__":
     import doctest
-    from grass.pygrass import utils
     from grass.pygrass.modules import Module
     Module("g.region", n=40, s=0, e=40, w=0, res=10)
     Module("r.mapcalc", expression="%s = row() + (10 * col())"%(test_raster_name),

lib/python/pygrass/vector/abstract.py

@@ -443,6 +443,5 @@ class Info(object):
 
 if __name__ == "__main__":
     import doctest
-    from grass.pygrass import utils
     utils.create_test_vector_map(test_vector_name)
     doctest.testmod()

lib/python/pygrass/vector/table.py

@@ -11,10 +11,6 @@ from __future__ import (nested_scopes, generators, division, absolute_import,
 import os
 import sys
 
-if sys.version_info.major == 3:
-    long = int
-    unicode = str
-
 import ctypes
 import numpy as np
 from sqlite3 import OperationalError
@@ -34,6 +30,12 @@ from grass.script.core import warning
 from grass.pygrass.vector import sql
 from grass.lib.ctypes_preamble import String
 
+
+if sys.version_info.major >= 3:
+    long = int
+    unicode = str
+
+
 # For test purposes
 test_vector_name = "table_doctest_map"
 

lib/python/pygrass/vector/testsuite/test_table.py

@@ -18,7 +18,7 @@ from grass.gunittest.main import test
 from grass.pygrass.vector.table import Table, get_path
 
 
-if sys.version_info.major == 3:
+if sys.version_info.major >= 3:
     long = int
 
 # dictionary that generate random data

lib/python/script/core.py

@@ -34,7 +34,7 @@ from .utils import KeyValue, parse_key_val, basename, encode, decode
 from grass.exceptions import ScriptError, CalledModuleError
 
 # PY2/PY3 compat
-if sys.version_info.major > 2:
+if sys.version_info.major >= 3:
     unicode = str
 
 # subprocess wrapper that uses shell on Windows
@@ -224,8 +224,9 @@ def shutil_which(cmd, mode=os.F_OK | os.X_OK, path=None):
                     return name
     return None
 
-if sys.version_info.major > 2:
-    shutil_which = shutil.which
+if sys.version_info.major >= 3:
+    # Use shutil.which in Python 3, not the custom implementation.
+    shutil_which = shutil.which  # noqa: F811
 
 # Added because of scripts calling scripts on MS Windows.
 # Module name (here cmd) differs from the file name (does not have extension).

lib/python/script/raster.py

@@ -29,7 +29,7 @@ from grass.exceptions import CalledModuleError
 from .utils import float_or_dms, parse_key_val, try_remove
 
 
-if sys.version_info.major == 3:
+if sys.version_info.major >= 3:
     unicode = str
 
 

lib/python/script/task.py

@@ -21,8 +21,9 @@ import re
 import sys
 import string
 
-if sys.version_info.major == 3:
-    unicode = str
+from .utils import encode, decode, split
+from .core import *
+
 
 try:
     import xml.etree.ElementTree as etree
@@ -36,8 +37,9 @@ if hasattr(etree, 'ParseError'):
 else:
     ETREE_EXCEPTIONS = (expat.ExpatError)
 
-from .utils import encode, decode, split
-from .core import *
+
+if sys.version_info.major >= 3:
+    unicode = str
 
 
 class grassTask:

lib/python/script/utils.py

@@ -27,7 +27,7 @@ import re
 import time
 
 
-if sys.version_info.major == 3:
+if sys.version_info.major >= 3:
     unicode = str
 
 
@@ -334,8 +334,13 @@ def split(s):
 def natural_sort(l):
     """Returns sorted strings using natural sort
     """
-    convert = lambda text: int(text) if text.isdigit() else text.lower()
-    alphanum_key = lambda key: [convert(c) for c in re.split('([0-9]+)', key)]
+
+    def convert(text):
+        return int(text) if text.isdigit() else text.lower()
+
+    def alphanum_key(key):
+        return [convert(c) for c in re.split('([0-9]+)', key)]
+
     return sorted(l, key=alphanum_key)
 
 

lib/python/temporal/core.py

@@ -33,9 +33,6 @@ import os
 import sys
 import grass.script as gscript
 
-if sys.version_info.major == 3:
-    long = int
-
 from .c_libraries_interface import *
 from grass.pygrass import messages
 from grass.script.utils import decode, encode
@@ -55,6 +52,9 @@ except:
 import atexit
 from datetime import datetime
 
+if sys.version_info.major >= 3:
+    long = int
+
 ###############################################################################
 
 

lib/python/temporal/space_time_datasets.py

@@ -24,7 +24,7 @@ from .temporal_extent import RasterAbsoluteTime, RasterRelativeTime, Raster3DAbs
     STRDSRelativeTime, STR3DSAbsoluteTime, STR3DSRelativeTime, STVDSAbsoluteTime, STVDSRelativeTime
 import grass.script.array as garray
 from .core import init
-from datetime import datetime
+
 
 ###############################################################################