From dd8785695b5b4469e8ea5d4d02791dba7b40ec5b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Edouard=20Choini=C3=A8re?= <27212526+echoix@users.noreply.github.com> Date: Sat, 4 Jan 2025 18:31:42 -0500 Subject: [PATCH] python: Various remaining pyrefact refactorings (#4911) * python: Factor out lines executed in both branches * style: Use parentheses when assigning tuples * style: Add parenthesis for clarity * python: Rewrite negation * style: Extract constant from loop * style: Membership test using a set * style: Factor out commonly used strings into constants * checks: Remove fixed Ruff exclusions --- pyproject.toml | 10 ++- python/grass/gunittest/case.py | 2 +- python/grass/gunittest/runner.py | 4 +- python/grass/imaging/images2gif.py | 25 +++--- python/grass/jupyter/setup.py | 4 +- python/grass/pygrass/modules/grid/grid.py | 3 +- python/grass/pygrass/utils.py | 2 +- python/grass/pygrass/vector/table.py | 10 +-- .../script/tests/grass_script_setup_test.py | 23 ++--- python/grass/temporal/mapcalc.py | 84 +++++-------------- .../spatial_topology_dataset_connector.py | 2 +- .../grass/temporal/temporal_vector_algebra.py | 4 +- 12 files changed, 66 insertions(+), 107 deletions(-) diff --git a/pyproject.toml b/pyproject.toml index aac843b84d2..2300282ad27 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -284,15 +284,15 @@ ignore = [ "gui/wxpython/icons/grass_icons.py" = ["PTH208"] "gui/wxpython/image2target/*.py" = ["SIM115"] "gui/wxpython/image2target/ii2t_manager.py" = ["PTH208"] -"gui/wxpython/iscatt/plots.py" = ["PLW0108"] "gui/wxpython/lmgr/workspace.py" = ["SIM115"] "gui/wxpython/location_wizard/wizard.py" = ["SIM115"] "gui/wxpython/mapdisp/main.py" = ["SIM115"] "gui/wxpython/modules/colorrules.py" = ["SIM115"] "gui/wxpython/modules/mcalc_builder.py" = ["SIM115"] -"gui/wxpython/photo2image/*.py" = ["SIM115"] -"gui/wxpython/psmap/*.py" = ["SIM115"] +"gui/wxpython/photo2image/ip2i_manager.py" = ["SIM115"] "gui/wxpython/psmap/dialogs.py" = ["PTH208"] +"gui/wxpython/psmap/frame.py" = ["SIM115"] +"gui/wxpython/psmap/instructions.py" = ["SIM115"] "gui/wxpython/psmap/utils.py" = ["PGH004"] "gui/wxpython/rdigit/controller.py" = ["SIM115"] "gui/wxpython/rlisetup/*.py" = ["SIM115"] @@ -301,7 +301,9 @@ ignore = [ "gui/wxpython/tools/update_menudata.py" = ["SIM115"] "gui/wxpython/tplot/frame.py" = ["FLY002"] "gui/wxpython/vdigit/mapwindow.py" = ["SIM115"] -"gui/wxpython/vnet/*.py" = ["SIM115"] +"gui/wxpython/vnet/vnet_core.py" = ["SIM115"] +"gui/wxpython/vnet/vnet_data.py" = ["SIM115"] +"gui/wxpython/vnet/widgets.py" = ["SIM115"] "gui/wxpython/web_services/dialogs.py" = ["SIM115"] "gui/wxpython/wxplot/profile.py" = ["A005", "SIM115"] "imagery/i.atcorr/create_iwave.py" = ["SIM115"] diff --git a/python/grass/gunittest/case.py b/python/grass/gunittest/case.py index e364f182fe8..601514c9cc9 100644 --- a/python/grass/gunittest/case.py +++ b/python/grass/gunittest/case.py @@ -1336,7 +1336,7 @@ def runModule(cls, module, expecting_stdout=False, **kwargs): module.name, module.get_python(), module.returncode, errors=errors ) # TODO: use this also in assert and apply when appropriate - if expecting_stdout and not module.outputs.stdout.strip(): + if expecting_stdout and (not module.outputs.stdout.strip()): if module.outputs.stderr: errors = " The errors are:\n" + module.outputs.stderr else: diff --git a/python/grass/gunittest/runner.py b/python/grass/gunittest/runner.py index dd1c72c615b..e27305cacbc 100644 --- a/python/grass/gunittest/runner.py +++ b/python/grass/gunittest/runner.py @@ -26,8 +26,8 @@ class _WritelnDecorator: def __init__(self, stream): self.stream = stream - def __getattr__(self, attr): - if attr in ("stream", "__getstate__"): + def __getattr__(self, attr: str): + if attr in {"stream", "__getstate__"}: raise AttributeError(attr) return getattr(self.stream, attr) diff --git a/python/grass/imaging/images2gif.py b/python/grass/imaging/images2gif.py index 545180b3a9c..89dfcb92eed 100644 --- a/python/grass/imaging/images2gif.py +++ b/python/grass/imaging/images2gif.py @@ -118,7 +118,7 @@ def checkImages(images): # Check and convert dtype if im.dtype == np.uint8: images2.append(im) # Ok - elif im.dtype in [np.float32, np.float64]: + elif im.dtype in (np.float32, np.float64): im = im.copy() im[im < 0] = 0 im[im > 1] = 1 @@ -296,7 +296,7 @@ def handleSubRectangles(self, images, subRectangles): images, xy = self.getSubRectangles(images) # Done - return images, xy + return (images, xy) def getSubRectangles(self, ims): """Calculate the minimal rectangles that need updating each frame. @@ -311,7 +311,7 @@ def getSubRectangles(self, ims): # Check image count if len(ims) < 2: - return ims, [(0, 0) for i in ims] + return (ims, [(0, 0) for i in ims]) # We need NumPy if np is None: @@ -334,11 +334,11 @@ def getSubRectangles(self, ims): Y = np.argwhere(diff.sum(1)) # Get rect coordinates if X.size and Y.size: - x0, x1 = int(X[0]), int(X[-1] + 1) - y0, y1 = int(Y[0]), int(Y[-1] + 1) + x0, x1 = (int(X[0]), int(X[-1] + 1)) + y0, y1 = (int(Y[0]), int(Y[-1] + 1)) else: # No change ... make it minimal - x0, x1 = 0, 2 - y0, y1 = 0, 2 + x0, x1 = (0, 2) + y0, y1 = (0, 2) # Cut out and store im2 = im[y0:y1, x0:x1] @@ -349,7 +349,7 @@ def getSubRectangles(self, ims): # Done # print('%1.2f seconds to determine subrectangles of %i images' % # (time.time()-t0, len(ims2))) - return ims2, xy + return (ims2, xy) def convertImagesToPIL(self, images, dither, nq=0): """Convert images to Paletted PIL images, which can then be @@ -373,7 +373,7 @@ def convertImagesToPIL(self, images, dither, nq=0): images2.append(im) # Convert to paletted PIL images - images, images2 = images2, [] + images, images2 = (images2, []) if nq >= 1: # NeuQuant algorithm for im in images: @@ -404,7 +404,7 @@ def writeGifToFile(self, fp, images, durations, loops, xys, disposes): """ # Obtain palette for all images and count each occurrence - palettes, occur = [], [] + palettes, occur = ([], []) for im in images: if not pillow: palette = getheader(im)[1] @@ -442,21 +442,20 @@ def writeGifToFile(self, fp, images, durations, loops, xys, disposes): # Write palette and image data # Gather info data = getdata(im) - imdes, data = data[0], data[1:] + imdes, data = (data[0], data[1:]) graphext = self.getGraphicsControlExt(durations[frames], disposes[frames]) # Make image descriptor suitable for using 256 local color palette lid = self.getImageDescriptor(im, xys[frames]) # Write local header + fp.write(graphext) if (palette != globalPalette) or (disposes[frames] != 2): # Use local color palette - fp.write(graphext) fp.write(lid) # write suitable image descriptor fp.write(palette) # write local color table fp.write("\x08") # LZW minimum size code else: # Use global color palette - fp.write(graphext) fp.write(imdes) # write suitable image descriptor # Write image data diff --git a/python/grass/jupyter/setup.py b/python/grass/jupyter/setup.py index 0ed64739039..0d7edec9767 100644 --- a/python/grass/jupyter/setup.py +++ b/python/grass/jupyter/setup.py @@ -87,8 +87,8 @@ def switch_mapset(self, path, location=None, mapset=None): gisenv = gs.gisenv() if ( not location - and not mapset - and len(Path(path).parts) == 1 + and (not mapset) + and (len(Path(path).parts) == 1) and mapset_exists( path=gisenv["GISDBASE"], location=gisenv["LOCATION_NAME"], mapset=path ) diff --git a/python/grass/pygrass/modules/grid/grid.py b/python/grass/pygrass/modules/grid/grid.py index cc3f406d72d..fd980766ba0 100644 --- a/python/grass/pygrass/modules/grid/grid.py +++ b/python/grass/pygrass/modules/grid/grid.py @@ -597,8 +597,9 @@ def get_works(self): if self.inlist: inms = {} cols = len(box_row) + + indx = row * cols + col for key in self.inlist: - indx = row * cols + col inms[key] = "%s@%s" % (self.inlist[key][indx], self.mset.name) # set the computational region, prepare the region parameters bbox = {k[0]: str(v) for k, v in box.items()[:-2]} diff --git a/python/grass/pygrass/utils.py b/python/grass/pygrass/utils.py index c9514c22c94..e66e421eeca 100644 --- a/python/grass/pygrass/utils.py +++ b/python/grass/pygrass/utils.py @@ -187,7 +187,7 @@ def is_clean_name(name) -> bool: False """ - return not libgis.G_legal_filename(name) < 0 + return libgis.G_legal_filename(name) >= 0 def coor2pixel(coord, region): diff --git a/python/grass/pygrass/vector/table.py b/python/grass/pygrass/vector/table.py index ee972a5b634..ac52d31baad 100644 --- a/python/grass/pygrass/vector/table.py +++ b/python/grass/pygrass/vector/table.py @@ -265,9 +265,9 @@ def update_odict(self): """Read columns name and types from table and update the odict attribute. """ + cur = self.conn.cursor() if self.is_pg(): # is a postgres connection - cur = self.conn.cursor() cur.execute("SELECT oid,typname FROM pg_type") diz = dict(cur.fetchall()) odict = OrderedDict() @@ -281,17 +281,15 @@ def update_odict(self): odict[name] = diz[ctype] except pg.ProgrammingError: pass - self.odict = odict else: # is a sqlite connection - cur = self.conn.cursor() cur.execute(sql.PRAGMA.format(tname=self.tname)) descr = cur.fetchall() odict = OrderedDict() for column in descr: name, ctype = column[1:3] odict[name] = ctype - self.odict = odict + self.odict = odict values = ",".join( [ "?", @@ -363,11 +361,9 @@ def names(self, remove=None, unicod=True): ['cat', 'name', 'value'] """ + nams = list(self.odict.keys()) if remove: - nams = list(self.odict.keys()) nams.remove(remove) - else: - nams = list(self.odict.keys()) if unicod: return nams return [str(name) for name in nams] diff --git a/python/grass/script/tests/grass_script_setup_test.py b/python/grass/script/tests/grass_script_setup_test.py index 36dfbb77dbb..f2593d1e0ca 100644 --- a/python/grass/script/tests/grass_script_setup_test.py +++ b/python/grass/script/tests/grass_script_setup_test.py @@ -8,6 +8,9 @@ import grass.script as gs +RUNTIME_GISBASE_SHOULD_BE_PRESENT = "Runtime (GISBASE) should be present" +SESSION_FILE_NOT_DELETED = "Session file not deleted" + xfail_mp_spawn = pytest.mark.xfail( multiprocessing.get_start_method() == "spawn", reason="Multiprocessing using 'spawn' start method requires pickable functions", @@ -91,8 +94,8 @@ def test_init_finish_global_functions_capture_strerr0_partial(tmp_path): ) session_file, runtime_present = run_in_subprocess(init_finish) assert session_file, "Expected file name from the subprocess" - assert runtime_present, "Runtime (GISBASE) should be present" - assert not os.path.exists(session_file), "Session file not deleted" + assert runtime_present, RUNTIME_GISBASE_SHOULD_BE_PRESENT + assert not os.path.exists(session_file), SESSION_FILE_NOT_DELETED @xfail_mp_spawn @@ -113,8 +116,8 @@ def init_finish(queue): session_file, runtime_present = run_in_subprocess(init_finish) assert session_file, "Expected file name from the subprocess" - assert runtime_present, "Runtime (GISBASE) should be present" - assert not os.path.exists(session_file), "Session file not deleted" + assert runtime_present, RUNTIME_GISBASE_SHOULD_BE_PRESENT + assert not os.path.exists(session_file), SESSION_FILE_NOT_DELETED @xfail_mp_spawn @@ -139,8 +142,8 @@ def init_finish(queue): init_finish ) assert session_file, "Expected file name from the subprocess" - assert runtime_present, "Runtime (GISBASE) should be present" - assert not os.path.exists(session_file), "Session file not deleted" + assert runtime_present, RUNTIME_GISBASE_SHOULD_BE_PRESENT + assert not os.path.exists(session_file), SESSION_FILE_NOT_DELETED # This is testing the current implementation behavior, but it is not required # to be this way in terms of design. assert runtime_present_after, "Runtime should continue to be present" @@ -189,7 +192,7 @@ def init_finish(queue): ) = run_in_subprocess(init_finish) # Runtime - assert runtime_present_during, "Runtime (GISBASE) should be present" + assert runtime_present_during, RUNTIME_GISBASE_SHOULD_BE_PRESENT # This is testing the current implementation behavior, but it is not required # to be this way in terms of design. assert runtime_present_after, "Expected GISBASE to be present when finished" @@ -198,7 +201,7 @@ def init_finish(queue): assert session_file_present_during, "Expected session file to be present" assert session_file_variable_present_during, "Variable GISRC should be present" assert not session_file_variable_present_after, "Not expecting GISRC when finished" - assert not os.path.exists(session_file), "Session file not deleted" + assert not os.path.exists(session_file), SESSION_FILE_NOT_DELETED @xfail_mp_spawn @@ -220,8 +223,8 @@ def workload(queue): session_file, file_existed, runtime_present = run_in_subprocess(workload) assert session_file, "Expected file name from the subprocess" assert file_existed, "File should have been present" - assert runtime_present, "Runtime (GISBASE) should be present" - assert not os.path.exists(session_file), "Session file not deleted" + assert runtime_present, RUNTIME_GISBASE_SHOULD_BE_PRESENT + assert not os.path.exists(session_file), SESSION_FILE_NOT_DELETED assert not os.environ.get("GISRC") assert not os.environ.get("GISBASE") diff --git a/python/grass/temporal/mapcalc.py b/python/grass/temporal/mapcalc.py index b1dbb5ce2db..68b2918170c 100644 --- a/python/grass/temporal/mapcalc.py +++ b/python/grass/temporal/mapcalc.py @@ -25,6 +25,9 @@ from .datetime_math import time_delta_to_relative_time from .open_stds import check_new_stds, open_new_stds, open_old_stds +_TEMPORAL_OPERATOR_SUPPORTS_ONLY_ABSOLUTE_TIME = ( + "The temporal operators <%s> support only absolute time." +) ############################################################################ @@ -507,74 +510,47 @@ def _parse_start_operators(expr, is_time_absolute: bool, current): if expr.find("start_year()") >= 0: if not is_time_absolute: - msgr.fatal( - _("The temporal operators <%s> support only absolute time.") - % ("start_*") - ) + msgr.fatal(_(_TEMPORAL_OPERATOR_SUPPORTS_ONLY_ABSOLUTE_TIME) % ("start_*")) expr = expr.replace("start_year()", str(start.year)) if expr.find("start_month()") >= 0: if not is_time_absolute: - msgr.fatal( - _("The temporal operators <%s> support only absolute time.") - % ("start_*") - ) + msgr.fatal(_(_TEMPORAL_OPERATOR_SUPPORTS_ONLY_ABSOLUTE_TIME) % ("start_*")) expr = expr.replace("start_month()", str(start.month)) if expr.find("start_week()") >= 0: if not is_time_absolute: - msgr.fatal( - _("The temporal operators <%s> support only absolute time.") - % ("start_*") - ) + msgr.fatal(_(_TEMPORAL_OPERATOR_SUPPORTS_ONLY_ABSOLUTE_TIME) % ("start_*")) expr = expr.replace("start_week()", str(start.isocalendar()[1])) if expr.find("start_day()") >= 0: if not is_time_absolute: - msgr.fatal( - _("The temporal operators <%s> support only absolute time.") - % ("start_*") - ) + msgr.fatal(_(_TEMPORAL_OPERATOR_SUPPORTS_ONLY_ABSOLUTE_TIME) % ("start_*")) expr = expr.replace("start_day()", str(start.day)) if expr.find("start_hour()") >= 0: if not is_time_absolute: - msgr.fatal( - _("The temporal operators <%s> support only absolute time.") - % ("start_*") - ) + msgr.fatal(_(_TEMPORAL_OPERATOR_SUPPORTS_ONLY_ABSOLUTE_TIME) % ("start_*")) expr = expr.replace("start_hour()", str(start.hour)) if expr.find("start_minute()") >= 0: if not is_time_absolute: - msgr.fatal( - _("The temporal operators <%s> support only absolute time.") - % ("start_*") - ) + msgr.fatal(_(_TEMPORAL_OPERATOR_SUPPORTS_ONLY_ABSOLUTE_TIME) % ("start_*")) expr = expr.replace("start_minute()", str(start.minute)) if expr.find("start_second()") >= 0: if not is_time_absolute: - msgr.fatal( - _("The temporal operators <%s> support only absolute time.") - % ("start_*") - ) + msgr.fatal(_(_TEMPORAL_OPERATOR_SUPPORTS_ONLY_ABSOLUTE_TIME) % ("start_*")) expr = expr.replace("start_second()", str(start.second)) if expr.find("start_dow()") >= 0: if not is_time_absolute: - msgr.fatal( - _("The temporal operators <%s> support only absolute time.") - % ("start_*") - ) + msgr.fatal(_(_TEMPORAL_OPERATOR_SUPPORTS_ONLY_ABSOLUTE_TIME) % ("start_*")) expr = expr.replace("start_dow()", str(start.isoweekday())) if expr.find("start_doy()") >= 0: if not is_time_absolute: - msgr.fatal( - _("The temporal operators <%s> support only absolute time.") - % ("start_*") - ) + msgr.fatal(_(_TEMPORAL_OPERATOR_SUPPORTS_ONLY_ABSOLUTE_TIME) % ("start_*")) year = datetime(start.year, 1, 1) delta = start - year @@ -609,9 +585,7 @@ def _parse_end_operators(expr, is_time_absolute: bool, current): if expr.find("end_year()") >= 0: if not is_time_absolute: - msgr.fatal( - _("The temporal operators <%s> support only absolute time.") % ("end_*") - ) + msgr.fatal(_(_TEMPORAL_OPERATOR_SUPPORTS_ONLY_ABSOLUTE_TIME) % ("end_*")) if not end: expr = expr.replace("end_year()", "null()") else: @@ -619,9 +593,7 @@ def _parse_end_operators(expr, is_time_absolute: bool, current): if expr.find("end_month()") >= 0: if not is_time_absolute: - msgr.fatal( - _("The temporal operators <%s> support only absolute time.") % ("end_*") - ) + msgr.fatal(_(_TEMPORAL_OPERATOR_SUPPORTS_ONLY_ABSOLUTE_TIME) % ("end_*")) if not end: expr = expr.replace("end_month()", "null()") else: @@ -629,9 +601,7 @@ def _parse_end_operators(expr, is_time_absolute: bool, current): if expr.find("end_week()") >= 0: if not is_time_absolute: - msgr.fatal( - _("The temporal operators <%s> support only absolute time.") % ("end_*") - ) + msgr.fatal(_(_TEMPORAL_OPERATOR_SUPPORTS_ONLY_ABSOLUTE_TIME) % ("end_*")) if not end: expr = expr.replace("end_week()", "null()") else: @@ -639,9 +609,7 @@ def _parse_end_operators(expr, is_time_absolute: bool, current): if expr.find("end_day()") >= 0: if not is_time_absolute: - msgr.fatal( - _("The temporal operators <%s> support only absolute time.") % ("end_*") - ) + msgr.fatal(_(_TEMPORAL_OPERATOR_SUPPORTS_ONLY_ABSOLUTE_TIME) % ("end_*")) if not end: expr = expr.replace("end_day()", "null()") else: @@ -649,9 +617,7 @@ def _parse_end_operators(expr, is_time_absolute: bool, current): if expr.find("end_hour()") >= 0: if not is_time_absolute: - msgr.fatal( - _("The temporal operators <%s> support only absolute time.") % ("end_*") - ) + msgr.fatal(_(_TEMPORAL_OPERATOR_SUPPORTS_ONLY_ABSOLUTE_TIME) % ("end_*")) if not end: expr = expr.replace("end_hour()", "null()") else: @@ -659,9 +625,7 @@ def _parse_end_operators(expr, is_time_absolute: bool, current): if expr.find("end_minute()") >= 0: if not is_time_absolute: - msgr.fatal( - _("The temporal operators <%s> support only absolute time.") % ("end_*") - ) + msgr.fatal(_(_TEMPORAL_OPERATOR_SUPPORTS_ONLY_ABSOLUTE_TIME) % ("end_*")) if not end: expr = expr.replace("end_minute()", "null()") else: @@ -669,9 +633,7 @@ def _parse_end_operators(expr, is_time_absolute: bool, current): if expr.find("end_second()") >= 0: if not is_time_absolute: - msgr.fatal( - _("The temporal operators <%s> support only absolute time.") % ("end_*") - ) + msgr.fatal(_(_TEMPORAL_OPERATOR_SUPPORTS_ONLY_ABSOLUTE_TIME) % ("end_*")) if not end: expr = expr.replace("end_second()", "null()") else: @@ -679,9 +641,7 @@ def _parse_end_operators(expr, is_time_absolute: bool, current): if expr.find("end_dow()") >= 0: if not is_time_absolute: - msgr.fatal( - _("The temporal operators <%s> support only absolute time.") % ("end_*") - ) + msgr.fatal(_(_TEMPORAL_OPERATOR_SUPPORTS_ONLY_ABSOLUTE_TIME) % ("end_*")) if not end: expr = expr.replace("end_dow()", "null()") else: @@ -689,9 +649,7 @@ def _parse_end_operators(expr, is_time_absolute: bool, current): if expr.find("end_doy()") >= 0: if not is_time_absolute: - msgr.fatal( - _("The temporal operators <%s> support only absolute time.") % ("end_*") - ) + msgr.fatal(_(_TEMPORAL_OPERATOR_SUPPORTS_ONLY_ABSOLUTE_TIME) % ("end_*")) if not end: expr = expr.replace("end_doy()", "null()") else: diff --git a/python/grass/temporal/spatial_topology_dataset_connector.py b/python/grass/temporal/spatial_topology_dataset_connector.py index 005ef2fcc45..7bd8d836f7b 100644 --- a/python/grass/temporal/spatial_topology_dataset_connector.py +++ b/python/grass/temporal/spatial_topology_dataset_connector.py @@ -296,7 +296,7 @@ def _generate_map_list_string(self, map_list, line_wrap: bool = True): count = 0 string = "" for map_ in map_list: - if line_wrap and count > 0 and count % 3 == 0: + if line_wrap and count > 0 and (count % 3 == 0): string += "\n | ............................ " count = 0 if count == 0: diff --git a/python/grass/temporal/temporal_vector_algebra.py b/python/grass/temporal/temporal_vector_algebra.py index 099a56b89e5..1cf981324f2 100644 --- a/python/grass/temporal/temporal_vector_algebra.py +++ b/python/grass/temporal/temporal_vector_algebra.py @@ -547,9 +547,9 @@ def p_statement_assign(self, t): ) for map_i in register_list: # Check if modules should be executed from command list. + map_i.load() if hasattr(map_i, "cmd_list") or hasattr(map_i, "is_new"): # Get meta data from grass database. - map_i.load() if map_i.is_in_db(dbif=dbif) and self.overwrite: # Update map in temporal database. map_i.update_all(dbif=dbif) @@ -568,7 +568,7 @@ def p_statement_assign(self, t): map_i.insert(dbif=dbif) else: # Map is original from an input STVDS - map_i.load() + pass # Register map in result space time dataset. if self.debug: print(map_i.get_temporal_extent_as_tuple())