]> git.openstreetmap.org Git - nominatim.git/commitdiff
Merge pull request #2136 from lonvia/introduce-pylint
authorSarah Hoffmann <lonvia@denofr.de>
Fri, 15 Jan 2021 13:39:26 +0000 (14:39 +0100)
committerGitHub <noreply@github.com>
Fri, 15 Jan 2021 13:39:26 +0000 (14:39 +0100)
Introduce pylint for code style checking for Python.

.github/workflows/ci-tests.yml
CMakeLists.txt
CONTRIBUTING.md
docs/admin/Installation.md
docs/develop/Development-Environment.md
nominatim/admin/exec_utils.py
nominatim/indexer/db.py
nominatim/nominatim.py

index e57431c012db86203e28158af873c95890025552..212707b32b4bdfa48668796199e36031e2283301 100644 (file)
@@ -47,12 +47,16 @@ jobs:
 
             - name: Install test prerequsites
               run: |
-                   sudo apt-get install -y -qq php-codesniffer
+                   sudo apt-get install -y -qq php-codesniffer pylint
                    sudo pip3 install behave
 
             - name: PHP linting
               run: phpcs --report-width=120 .
 
+            - name: Python linting
+              run: |
+                   pylint nominatim
+
             - name: PHP unit tests
               run: phpunit ./
               working-directory: test/php
index 1c2745946bcb133a85cf5a95ca24b06a4cd9e36e..761531b10218ea5e7feda79c884de2841fc143bd 100644 (file)
@@ -61,7 +61,7 @@ endif()
 #-----------------------------------------------------------------------------
 
 if (BUILD_IMPORTER)
-    find_package(PythonInterp 3)
+    find_package(PythonInterp 3.5 REQUIRED)
 
     find_program(PYOSMIUM pyosmium-get-changes)
     if (NOT EXISTS "${PYOSMIUM}")
@@ -140,6 +140,8 @@ if (BUILD_TESTS)
 
     set(TEST_BDD db osm2pgsql api)
 
+    find_program(PYLINT NAMES pylint3 pylint)
+
     foreach (test ${TEST_BDD})
         add_test(NAME bdd_${test}
                  COMMAND behave ${test}
@@ -155,6 +157,15 @@ if (BUILD_TESTS)
     add_test(NAME phpcs
              COMMAND phpcs --report-width=120 --colors lib website utils
              WORKING_DIRECTORY ${PROJECT_SOURCE_DIR})
+
+    if (PYLINT)
+        message(STATUS "Using '${PYLINT}' for Python linting.")
+        add_test(NAME pylint
+                 COMMAND ${PYLINT} nominatim
+                 WORKING_DIRECTORY ${PROJECT_SOURCE_DIR})
+    else()
+        message(STATUS "pylint not found. Linting tests disabled.")
+    endif()
 endif()
 
 #-----------------------------------------------------------------------------
index a61456671986c138d722cbbc17ef67915af4d7a5..552d1da1d285d0676509e385d982e3f1802574b5 100644 (file)
@@ -49,10 +49,11 @@ are in process of consolidating the style. The following rules apply:
  * for PHP variables use CamelCase with a prefixing letter indicating the type
    (i - integer, f - float, a - array, s - string, o - object)
 
-The coding style is enforced with PHPCS and can be tested with:
+The coding style is enforced with PHPCS and pylint. It can be tested with:
 
 ```
-  phpcs --report-width=120 --colors .
+phpcs --report-width=120 --colors .
+pylint3 nominatim
 ```
 
 ## Testing
index c9d000b22ae8b85c05f22c5cb9a7c3f8507f5249..d8c98ef5056dfecb1a6b2b4761214eee8e8f22a9 100644 (file)
@@ -38,7 +38,7 @@ For running Nominatim:
 
   * [PostgreSQL](https://www.postgresql.org) (9.3+)
   * [PostGIS](https://postgis.net) (2.2+)
-  * [Python 3](https://www.python.org/) (3.4+)
+  * [Python 3](https://www.python.org/) (3.5+)
   * [Psycopg2](https://www.psycopg.org)
   * [PHP](https://php.net) (7.0 or later)
   * PHP-pgsql
index 86df1fb9661f61545d7d2142eeb78d6783eb4e3c..75324e716f58829f07e4717718df33c5dbee9771 100644 (file)
@@ -31,6 +31,7 @@ unit tests (using PHPUnit). It has the following additional requirements:
 * [behave test framework](https://behave.readthedocs.io) >= 1.2.5
 * [phpunit](https://phpunit.de) >= 7.3
 * [PHP CodeSniffer](https://github.com/squizlabs/PHP_CodeSniffer)
+* [Pylint](https://pylint.org/)
 
 The documentation is built with mkdocs:
 
@@ -46,7 +47,7 @@ To install all necessary packages run:
 
 ```sh
 sudo apt install php-cgi phpunit php-codesniffer \
-                 python3-pip python3-setuptools python3-dev
+                 python3-pip python3-setuptools python3-dev pylint
 
 pip3 install --user behave mkdocs
 ```
index f3f59dea9cbd26c73670da1f1ae1586540ee8f32..0158227957f04bc6afa76f820e276672213a1e7f 100644 (file)
@@ -21,9 +21,7 @@ def run_legacy_script(script, *args, nominatim_env=None, throw_on_fail=False):
     if not env['NOMINATIM_OSM2PGSQL_BINARY']:
         env['NOMINATIM_OSM2PGSQL_BINARY'] = nominatim_env.osm2pgsql_path
 
-    proc = subprocess.run(cmd, cwd=str(nominatim_env.project_dir), env=env)
-
-    if throw_on_fail:
-        proc.check_returncode()
+    proc = subprocess.run(cmd, cwd=str(nominatim_env.project_dir), env=env,
+                          check=throw_on_fail)
 
     return proc.returncode
index 037c3fb2203c7d3a27d112e1eef3d9f6e69fd957..85b844312ba271a638be8ef084dbe34a9274889d 100644 (file)
@@ -1,15 +1,19 @@
 # SPDX-License-Identifier: GPL-2.0-only
 #
 # This file is part of Nominatim.
-# Copyright (C) 2020 Sarah Hoffmann
-
+# Copyright (C) 2021 by the Nominatim developer community.
+# For a full list of authors see the git log.
+""" Database helper functions for the indexer.
+"""
 import logging
 import psycopg2
 from psycopg2.extras import wait_select
 
-log = logging.getLogger()
+LOG = logging.getLogger()
 
 def make_connection(options, asynchronous=False):
+    """ Create a psycopg2 connection from the given options.
+    """
     params = {'dbname' : options.dbname,
               'user' : options.user,
               'password' : options.password,
@@ -19,7 +23,7 @@ def make_connection(options, asynchronous=False):
 
     return psycopg2.connect(**params)
 
-class DBConnection(object):
+class DBConnection:
     """ A single non-blocking database connection.
     """
 
@@ -29,6 +33,7 @@ class DBConnection(object):
         self.options = options
 
         self.conn = None
+        self.cursor = None
         self.connect()
 
     def connect(self):
@@ -51,7 +56,7 @@ class DBConnection(object):
         # implemented.
         self.perform(
             """ UPDATE pg_settings SET setting = -1 WHERE name = 'jit_above_cost';
-                UPDATE pg_settings SET setting = 0 
+                UPDATE pg_settings SET setting = 0
                    WHERE name = 'max_parallel_workers_per_gather';""")
         self.wait()
 
@@ -63,14 +68,14 @@ class DBConnection(object):
                 wait_select(self.conn)
                 self.current_query = None
                 return
-            except psycopg2.extensions.TransactionRollbackError as e:
-                if e.pgcode == '40P01':
-                    log.info("Deadlock detected (params = {}), retry."
-                              .format(self.current_params))
+            except psycopg2.extensions.TransactionRollbackError as error:
+                if error.pgcode == '40P01':
+                    LOG.info("Deadlock detected (params = %s), retry.",
+                             str(self.current_params))
                     self.cursor.execute(self.current_query, self.current_params)
                 else:
                     raise
-            except psycopg2.errors.DeadlockDetected:
+            except psycopg2.errors.DeadlockDetected: # pylint: disable=E1101
                 self.cursor.execute(self.current_query, self.current_params)
 
     def perform(self, sql, args=None):
@@ -99,14 +104,13 @@ class DBConnection(object):
             if self.conn.poll() == psycopg2.extensions.POLL_OK:
                 self.current_query = None
                 return True
-        except psycopg2.extensions.TransactionRollbackError as e:
-            if e.pgcode == '40P01':
-                log.info("Deadlock detected (params = {}), retry.".format(self.current_params))
+        except psycopg2.extensions.TransactionRollbackError as error:
+            if error.pgcode == '40P01':
+                LOG.info("Deadlock detected (params = %s), retry.", str(self.current_params))
                 self.cursor.execute(self.current_query, self.current_params)
             else:
                 raise
-        except psycopg2.errors.DeadlockDetected:
+        except psycopg2.errors.DeadlockDetected: # pylint: disable=E1101
             self.cursor.execute(self.current_query, self.current_params)
 
         return False
-
index b20673d2a0fbef3c71ff2f2d93af811f8df2062d..8cac583e663241622a77d228b16fcc9f4a22baa5 100755 (executable)
 # along with this program; if not, write to the Free Software
 # Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301, USA.
 #-----------------------------------------------------------------------------
-
-from argparse import ArgumentParser, RawDescriptionHelpFormatter, ArgumentTypeError
+# pylint: disable=C0111
+from argparse import ArgumentParser, RawDescriptionHelpFormatter
 import logging
 import sys
-import re
 import getpass
-from datetime import datetime
 import select
 
-from indexer.progress import ProgressLogger
-from indexer.db import DBConnection, make_connection
+from indexer.progress import ProgressLogger # pylint: disable=E0401
+from indexer.db import DBConnection, make_connection # pylint: disable=E0401
 
-log = logging.getLogger()
+LOG = logging.getLogger()
 
-class RankRunner(object):
+class RankRunner:
     """ Returns SQL commands for indexing one rank within the placex table.
     """
 
@@ -55,34 +53,39 @@ class RankRunner(object):
                   WHERE indexed_status > 0 and rank_address = {}
                   ORDER BY geometry_sector""".format(self.rank)
 
-    def sql_index_place(self, ids):
+    @staticmethod
+    def sql_index_place(ids):
         return "UPDATE placex SET indexed_status = 0 WHERE place_id IN ({})"\
                .format(','.join((str(i) for i in ids)))
 
 
-class InterpolationRunner(object):
+class InterpolationRunner:
     """ Returns SQL commands for indexing the address interpolation table
         location_property_osmline.
     """
 
-    def name(self):
+    @staticmethod
+    def name():
         return "interpolation lines (location_property_osmline)"
 
-    def sql_count_objects(self):
+    @staticmethod
+    def sql_count_objects():
         return """SELECT count(*) FROM location_property_osmline
                   WHERE indexed_status > 0"""
 
-    def sql_get_objects(self):
+    @staticmethod
+    def sql_get_objects():
         return """SELECT place_id FROM location_property_osmline
                   WHERE indexed_status > 0
                   ORDER BY geometry_sector"""
 
-    def sql_index_place(self, ids):
+    @staticmethod
+    def sql_index_place(ids):
         return """UPDATE location_property_osmline
                   SET indexed_status = 0 WHERE place_id IN ({})"""\
                .format(','.join((str(i) for i in ids)))
 
-class BoundaryRunner(object):
+class BoundaryRunner:
     """ Returns SQL commands for indexing the administrative boundaries
         of a certain rank.
     """
@@ -105,23 +108,24 @@ class BoundaryRunner(object):
                         and class = 'boundary' and type = 'administrative'
                   ORDER BY partition, admin_level""".format(self.rank)
 
-    def sql_index_place(self, ids):
+    @staticmethod
+    def sql_index_place(ids):
         return "UPDATE placex SET indexed_status = 0 WHERE place_id IN ({})"\
                .format(','.join((str(i) for i in ids)))
 
-class Indexer(object):
+class Indexer:
     """ Main indexing routine.
     """
 
-    def __init__(self, options):
-        self.minrank = max(1, options.minrank)
-        self.maxrank = min(30, options.maxrank)
-        self.conn = make_connection(options)
-        self.threads = [DBConnection(options) for i in range(options.threads)]
+    def __init__(self, opts):
+        self.minrank = max(1, opts.minrank)
+        self.maxrank = min(30, opts.maxrank)
+        self.conn = make_connection(opts)
+        self.threads = [DBConnection(opts) for _ in range(opts.threads)]
 
     def index_boundaries(self):
-        log.warning("Starting indexing boundaries using {} threads".format(
-                      len(self.threads)))
+        LOG.warning("Starting indexing boundaries using %s threads",
+                    len(self.threads))
 
         for rank in range(max(self.minrank, 5), min(self.maxrank, 26)):
             self.index(BoundaryRunner(rank))
@@ -129,8 +133,8 @@ class Indexer(object):
     def index_by_rank(self):
         """ Run classic indexing by rank.
         """
-        log.warning("Starting indexing rank ({} to {}) using {} threads".format(
-                 self.minrank, self.maxrank, len(self.threads)))
+        LOG.warning("Starting indexing rank (%i to %i) using %i threads",
+                    self.minrank, self.maxrank, len(self.threads))
 
         for rank in range(max(1, self.minrank), self.maxrank):
             self.index(RankRunner(rank))
@@ -147,13 +151,13 @@ class Indexer(object):
             for indexing. `batch` describes the number of objects that
             should be processed with a single SQL statement
         """
-        log.warning("Starting %s (using batch size %s)", obj.name(), batch)
+        LOG.warning("Starting %s (using batch size %s)", obj.name(), batch)
 
         cur = self.conn.cursor()
         cur.execute(obj.sql_count_objects())
 
         total_tuples = cur.fetchone()[0]
-        log.debug("Total number of rows: {}".format(total_tuples))
+        LOG.debug("Total number of rows: %i", total_tuples)
 
         cur.close()
 
@@ -166,10 +170,10 @@ class Indexer(object):
             next_thread = self.find_free_thread()
             while True:
                 places = [p[0] for p in cur.fetchmany(batch)]
-                if len(places) == 0:
+                if not places:
                     break
 
-                log.debug("Processing places: {}".format(places))
+                LOG.debug("Processing places: %s", str(places))
                 thread = next(next_thread)
 
                 thread.perform(obj.sql_index_place(places))
@@ -177,8 +181,8 @@ class Indexer(object):
 
             cur.close()
 
-            for t in self.threads:
-                t.wait()
+            for thread in self.threads:
+                thread.wait()
 
         progress.done()
 
@@ -198,10 +202,10 @@ class Indexer(object):
             # refresh the connections occasionaly to avoid potential
             # memory leaks in Postgresql.
             if command_stat > 100000:
-                for t in self.threads:
-                    while not t.is_done():
-                        t.wait()
-                    t.connect()
+                for thread in self.threads:
+                    while not thread.is_done():
+                        thread.wait()
+                    thread.connect()
                 command_stat = 0
                 ready = self.threads
             else:
@@ -213,58 +217,55 @@ class Indexer(object):
 def nominatim_arg_parser():
     """ Setup the command-line parser for the tool.
     """
-    def h(s):
-        return re.sub("\s\s+" , " ", s)
-
-    p = ArgumentParser(description="Indexing tool for Nominatim.",
-                       formatter_class=RawDescriptionHelpFormatter)
-
-    p.add_argument('-d', '--database',
-                   dest='dbname', action='store', default='nominatim',
-                   help='Name of the PostgreSQL database to connect to.')
-    p.add_argument('-U', '--username',
-                   dest='user', action='store',
-                   help='PostgreSQL user name.')
-    p.add_argument('-W', '--password',
-                   dest='password_prompt', action='store_true',
-                   help='Force password prompt.')
-    p.add_argument('-H', '--host',
-                   dest='host', action='store',
-                   help='PostgreSQL server hostname or socket location.')
-    p.add_argument('-P', '--port',
-                   dest='port', action='store',
-                   help='PostgreSQL server port')
-    p.add_argument('-b', '--boundary-only',
-                   dest='boundary_only', action='store_true',
-                   help='Only index administrative boundaries (ignores min/maxrank).')
-    p.add_argument('-r', '--minrank',
-                   dest='minrank', type=int, metavar='RANK', default=0,
-                   help='Minimum/starting rank.')
-    p.add_argument('-R', '--maxrank',
-                   dest='maxrank', type=int, metavar='RANK', default=30,
-                   help='Maximum/finishing rank.')
-    p.add_argument('-t', '--threads',
-                   dest='threads', type=int, metavar='NUM', default=1,
-                   help='Number of threads to create for indexing.')
-    p.add_argument('-v', '--verbose',
-                   dest='loglevel', action='count', default=0,
-                   help='Increase verbosity')
-
-    return p
+    parser = ArgumentParser(description="Indexing tool for Nominatim.",
+                            formatter_class=RawDescriptionHelpFormatter)
+
+    parser.add_argument('-d', '--database',
+                        dest='dbname', action='store', default='nominatim',
+                        help='Name of the PostgreSQL database to connect to.')
+    parser.add_argument('-U', '--username',
+                        dest='user', action='store',
+                        help='PostgreSQL user name.')
+    parser.add_argument('-W', '--password',
+                        dest='password_prompt', action='store_true',
+                        help='Force password prompt.')
+    parser.add_argument('-H', '--host',
+                        dest='host', action='store',
+                        help='PostgreSQL server hostname or socket location.')
+    parser.add_argument('-P', '--port',
+                        dest='port', action='store',
+                        help='PostgreSQL server port')
+    parser.add_argument('-b', '--boundary-only',
+                        dest='boundary_only', action='store_true',
+                        help='Only index administrative boundaries (ignores min/maxrank).')
+    parser.add_argument('-r', '--minrank',
+                        dest='minrank', type=int, metavar='RANK', default=0,
+                        help='Minimum/starting rank.')
+    parser.add_argument('-R', '--maxrank',
+                        dest='maxrank', type=int, metavar='RANK', default=30,
+                        help='Maximum/finishing rank.')
+    parser.add_argument('-t', '--threads',
+                        dest='threads', type=int, metavar='NUM', default=1,
+                        help='Number of threads to create for indexing.')
+    parser.add_argument('-v', '--verbose',
+                        dest='loglevel', action='count', default=0,
+                        help='Increase verbosity')
+
+    return parser
 
 if __name__ == '__main__':
     logging.basicConfig(stream=sys.stderr, format='%(levelname)s: %(message)s')
 
-    options = nominatim_arg_parser().parse_args(sys.argv[1:])
+    OPTIONS = nominatim_arg_parser().parse_args(sys.argv[1:])
 
-    log.setLevel(max(3 - options.loglevel, 0) * 10)
+    LOG.setLevel(max(3 - OPTIONS.loglevel, 0) * 10)
 
-    options.password = None
-    if options.password_prompt:
-        password = getpass.getpass("Database password: ")
-        options.password = password
+    OPTIONS.password = None
+    if OPTIONS.password_prompt:
+        PASSWORD = getpass.getpass("Database password: ")
+        OPTIONS.password = PASSWORD
 
-    if options.boundary_only:
-        Indexer(options).index_boundaries()
+    if OPTIONS.boundary_only:
+        Indexer(OPTIONS).index_boundaries()
     else:
-        Indexer(options).index_by_rank()
+        Indexer(OPTIONS).index_by_rank()