Skip to content

style: Fix raise-without-from-inside-except (B904) #4032

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion gui/wxpython/animation/data.py
Original file line number Diff line number Diff line change
Expand Up @@ -301,7 +301,7 @@ def SetName(self, name):
name = validateTimeseriesName(name, self._mapType)
self._maps = getRegisteredMaps(name, self._mapType)
except (GException, ScriptError) as e:
raise ValueError(str(e))
raise ValueError(str(e)) from e
else:
self._maps = validateMapNames(name.split(","), self._mapType)
self._name = name
Expand Down
4 changes: 2 additions & 2 deletions gui/wxpython/animation/nviztask.py
Original file line number Diff line number Diff line change
Expand Up @@ -34,14 +34,14 @@ def Load(self, filename):
self.filename = filename
try:
gxwXml = ProcessWorkspaceFile(ET.parse(self.filename))
except Exception:
except Exception as e:
raise GException(
_(
"Reading workspace file <%s> failed.\n"
"Invalid file, unable to parse XML document."
)
% filename
)
) from e
# for display in gxwXml.displays:
# pprint(display)
# for layer in gxwXml.layers:
Expand Down
8 changes: 4 additions & 4 deletions gui/wxpython/core/settings.py
Original file line number Diff line number Diff line change
Expand Up @@ -992,12 +992,12 @@ def SaveToFile(self, settings=None):
with open(self.filePath, "w") as f:
json.dump(settings, f, indent=2, cls=SettingsJSONEncoder)
except OSError as e:
raise GException(e)
raise GException(e) from e
except Exception as e:
raise GException(
_("Writing settings to file <%(file)s> failed.\n\nDetails: %(detail)s")
% {"file": self.filePath, "detail": e}
)
) from e
return self.filePath

def _parseValue(self, value, read=False):
Expand Down Expand Up @@ -1099,10 +1099,10 @@ def Set(self, group, value, key=None, subkey=None, settings_type="user"):
settings[group][key][subkey] = value
return

except KeyError:
except KeyError as e:
raise GException(
"%s '%s:%s:%s'" % (_("Unable to set "), group, key, subkey)
)
) from e

def Append(self, dict, group, key, subkey, value, overwrite=True):
"""Set value of key/subkey
Expand Down
4 changes: 2 additions & 2 deletions gui/wxpython/core/utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -476,8 +476,8 @@ def __ll_parts(value, reverse=False, precision=3):
d = d[:-1]
m = "0"
s = "0.0"
except ValueError:
raise ValueError
except ValueError as err:
raise ValueError from err

if hs not in {"N", "S", "E", "W"}:
raise ValueError
Expand Down
10 changes: 5 additions & 5 deletions gui/wxpython/dbmgr/base.py
Original file line number Diff line number Diff line change
Expand Up @@ -179,15 +179,15 @@ def LoadData(self, layer, columns=None, where=None, sql=None):
keyColumn = self.mapDBInfo.layers[layer]["key"]
try:
self.columns = self.mapDBInfo.tables[tableName]
except KeyError:
except KeyError as err:
raise GException(
_(
"Attribute table <%s> not found. "
"For creating the table switch to "
"'Manage layers' tab."
)
% tableName
)
) from err

if not columns:
columns = self.mapDBInfo.GetColumns(tableName)
Expand Down Expand Up @@ -1559,7 +1559,7 @@ def OnDataItemEdit(self, event):
raise ValueError(
_("Value '%(value)s' needs to be entered as %(type)s.")
% {"value": str(values[i]), "type": column["type"]}
)
) from None

if column["ctype"] == str:
if "'" in values[i]: # replace "'" -> "''"
Expand Down Expand Up @@ -1677,14 +1677,14 @@ def OnDataItemAdd(self, event):
"value": values[i],
"type": tlist.columns[columnName[i]]["type"],
}
)
) from None
except KeyError:
raise KeyError(
_("Column '%(column)s' does not exist.")
% {
"column": columnName[i],
}
)
) from None
columnsString += "%s," % columnName[i]

if tlist.columns[columnName[i]]["ctype"] == str:
Expand Down
2 changes: 1 addition & 1 deletion gui/wxpython/gmodeler/model.py
Original file line number Diff line number Diff line change
Expand Up @@ -322,7 +322,7 @@ def LoadModel(self, filename):
gxmXml = ProcessModelFile(ET.parse(filename))
except Exception as e:
msg = "{}".format(e)
raise GException(msg)
raise GException(msg) from e

if self.canvas:
win = self.canvas.parent
Expand Down
6 changes: 3 additions & 3 deletions gui/wxpython/gui_core/forms.py
Original file line number Diff line number Diff line change
Expand Up @@ -3080,7 +3080,7 @@ def ParseCommand(self, cmd, completed=None):
global _blackList
self.grass_task = gtask.parse_interface(cmd[0], blackList=_blackList)
except (ScriptError, ValueError) as e:
raise gcmd.GException(e.value)
raise gcmd.GException(e.value) from e

# if layer parameters previously set, re-insert them into dialog
if completed is not None:
Expand Down Expand Up @@ -3110,15 +3110,15 @@ def ParseCommand(self, cmd, completed=None):
# parameter
try:
key, value = option.split("=", 1)
except ValueError:
except ValueError as e:
if self.grass_task.firstParam:
if i == 0: # add key name of first parameter if not given
key = self.grass_task.firstParam
value = option
else:
raise gcmd.GException(
_("Unable to parse command '%s'") % " ".join(cmd)
)
) from e
else:
continue

Expand Down
5 changes: 2 additions & 3 deletions gui/wxpython/gui_core/toolbars.py
Original file line number Diff line number Diff line change
Expand Up @@ -224,11 +224,10 @@ def Enable(self, tool, enable=True):
id = getattr(self.widget, tool[0])
else:
id = getattr(self.widget, tool)
except AttributeError:
except AttributeError as e:
# TODO: test everything that this is not raised
# this error was ignored for a long time
raise AttributeError("Toolbar does not have a tool %s." % tool)
return
raise AttributeError("Toolbar does not have a tool %s." % tool) from e

self.classObject.EnableTool(self.widget, id, enable)

Expand Down
2 changes: 1 addition & 1 deletion gui/wxpython/iscatt/plots.py
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@
'The Scatterplot Tool needs the "matplotlib" '
"(python-matplotlib) package to be installed. {0}"
).format(e)
)
) from e

import grass.script as gs
from grass.pydispatch.signal import Signal
Expand Down
2 changes: 1 addition & 1 deletion gui/wxpython/mapdisp/main.py
Original file line number Diff line number Diff line change
Expand Up @@ -364,7 +364,7 @@ def __next__(self):
try:
result = items[self._index]
except IndexError:
raise StopIteration
raise StopIteration from None
self._index += 1
return result

Expand Down
8 changes: 4 additions & 4 deletions gui/wxpython/rdigit/controller.py
Original file line number Diff line number Diff line change
Expand Up @@ -467,8 +467,8 @@ def _createNewMap(self, mapName, backgroundMap, mapType):
).strip()
if values:
self.uploadMapCategories.emit(values=values.split("\n"))
except CalledModuleError:
raise ScriptError
except CalledModuleError as e:
raise ScriptError(e.msg) from e
self._backupRaster(name)

name = name + "@" + gcore.gisenv()["MAPSET"]
Expand All @@ -494,8 +494,8 @@ def _backupRaster(self, name):
backup = name + "_backupcopy_" + str(os.getpid())
try:
gcore.run_command("g.copy", raster=[name, backup], quiet=True)
except CalledModuleError:
raise ScriptError
except CalledModuleError as e:
raise ScriptError(e.msg) from e

self._backupRasterName = backup

Expand Down
2 changes: 1 addition & 1 deletion gui/wxpython/timeline/frame.py
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@
"(python-matplotlib and on some systems also python-matplotlib-wx) "
"package(s) to be installed. {}"
).format(e)
)
) from e

import grass.script as gs

Expand Down
6 changes: 3 additions & 3 deletions gui/wxpython/tplot/frame.py
Original file line number Diff line number Diff line change
Expand Up @@ -46,9 +46,9 @@
raise ImportError(
_(
'The Temporal Plot Tool needs the "matplotlib" '
"(python-matplotlib) package to be installed. {0}"
).format(e)
)
"(python-matplotlib) package to be installed."
)
) from e
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried to understand the issue. Not sure if I got it completly right. My interpretation would be, that authors did not want excepting-chaining when they included the previous exception text with format in the raised exception...
So from e would print e twice in this case, no? So probably, from None is the right choice when the original Error message is included in the raised exception? Not sure when one should raise a python error instead of a eg. gs.fatal(())_ ... Maybe @wenzeslaus has some thoughts on exception chaining?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think exception chaining was used at all before. We're way more precise about our python usage now than when the python code started, so I'm pretty sure not everything was thoroughly though out before.

But for this case, maybe forgetting the .format(e) and using exception chaining for the details is in the same spririt?
The spirit (as I understand), was to have better error messages that what python was giving at the time. Now, especially in the latests versions, have been giving better error messages, so it's less frightening and more useful. I think there's value of having the real errors showed up (as we will be having that in the logs in the issues the users will file), and we might want to have all that extra context.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, it's only a hint for an import error. So that's kinda easy and has no real implications for the logic of the code itself. It's only a nice-to-have hint.

So, I'm suggesting exception chaining without the format(e) here, it will give something similar to the example of python docs:

>>> def func():
...    raise ConnectionError
... 
>>> try:
...     func()
... except ConnectionError as exc:
...     raise RuntimeError('Failed to open database') from exc
... 
Traceback (most recent call last):
  File "<stdin>", line 2, in <module>
  File "<stdin>", line 2, in func
ConnectionError

The above exception was the direct cause of the following exception:

Traceback (most recent call last):
  File "<stdin>", line 4, in <module>
RuntimeError: Failed to open database
>>> def func():
...    raise ConnectionError
... 
>>> try:
...     func()
... except ConnectionError as exc:
...     raise RuntimeError('Failed to open database') from exc
... 
Traceback (most recent call last):
  File "<stdin>", line 2, in <module>
  File "<stdin>", line 2, in func
ConnectionError

The above exception was the direct cause of the following exception:

Traceback (most recent call last):
  File "<stdin>", line 4, in <module>
RuntimeError: Failed to open database
try:
    import matplotlib as mpl
    # The recommended way to use wx with mpl is with the WXAgg
    # backend.
    mpl.use("WXAgg")
    from matplotlib.figure import Figure
    from matplotlib.backends.backend_wxagg import (
        FigureCanvasWxAgg as FigCanvas,
        NavigationToolbar2WxAgg as NavigationToolbar,
    )
    import matplotlib.dates as mdates
except ImportError as e:
    raise ImportError(
        _(
            'The Temporal Plot Tool needs the "matplotlib" '
            "(python-matplotlib) package to be installed."
        )
    ) from e

giving (except for the line numbers and file names, I wrote it by hand):

Traceback (most recent call last):
  File "<stdin>", line 2, in <module>
  File "<stdin>", line 2, in func
ImportError

The above exception was the direct cause of the following exception:

Traceback (most recent call last):
  File "<stdin>", line 4, in <module>
ImportError: The Temporal Plot Tool needs the "matplotlib" (python-matplotlib) package to be installed.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Both exception chaining and exception notes seem OK to me. However, I tend to think that all tracebacks also could be considered bugs, and when we catch an exception with try except, a simple (error-) message should be given that is addressed to the user and not to a developer who should be able to read traceback...
Maybe this discussion would make a good startingpoint to improve the exception-handling section in the style-guide?!?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As for dependencies, see also https://trac.osgeo.org/grass/ticket/2895

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Even then, that ticket is starting to get old, now everything should be in a pyproject.toml. For addons, I think each should be an individual "real" python package, each with a pyproject.toml, and could be fetched only for that subdirectory and work perfectly (and allow code tools to understand that structure).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Making addons python packages is also the way QGIS is going, I think. However, we also have C-addons and addons using R...

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't go with the TODO finally. Still working on the next PR, the unsafe fixes are a little longer to go through, and I have to do more manual tuning.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@neteler do you have any opions/thoughts on raised exceptions, exception chaining vs. GRASS messages (fatal/error)?
See also my comment here: #4032 (comment)



import grass.temporal as tgis
Expand Down
2 changes: 1 addition & 1 deletion gui/wxpython/wxgui.py
Original file line number Diff line number Diff line change
Expand Up @@ -151,7 +151,7 @@ def main(argv=None):
try:
opts, args = getopt.getopt(argv[1:], "hw:", ["help", "workspace"])
except getopt.error as msg:
raise Usage(msg)
raise Usage(msg) from None
except Usage as err:
print(err.msg, file=sys.stderr)
print(sys.stderr, "for help use --help", file=sys.stderr)
Expand Down
1 change: 0 additions & 1 deletion pyproject.toml
Original file line number Diff line number Diff line change
Expand Up @@ -114,7 +114,6 @@ ignore = [
"B026", # star-arg-unpacking-after-keyword-arg
"B028", # no-explicit-stacklevel
"B034", # re-sub-positional-args
"B904", # raise-without-from-inside-except
"B909", # loop-iterator-mutation
"BLE001", # blind-except
"C414", # unnecessary-double-cast-or-process
Expand Down
2 changes: 1 addition & 1 deletion python/grass/gunittest/case.py
Original file line number Diff line number Diff line change
Expand Up @@ -1334,7 +1334,7 @@ def runModule(cls, module, expecting_stdout=False, **kwargs):
# TODO: message format, parameters
raise CalledModuleError(
module.name, module.get_python(), module.returncode, errors=errors
)
) from None
# TODO: use this also in assert and apply when appropriate
if expecting_stdout and (not module.outputs.stdout.strip()):
if module.outputs.stderr:
Expand Down
2 changes: 1 addition & 1 deletion python/grass/gunittest/loader.py
Original file line number Diff line number Diff line change
Expand Up @@ -180,7 +180,7 @@ def discover_modules(
raise ImportError(
"Cannot import module named %s in %s (%s)"
% (name, full, e.message)
)
) from e
# alternative is to create TestClass which will raise
# see unittest.loader
if add:
Expand Down
4 changes: 2 additions & 2 deletions python/grass/imaging/images2avi.py
Original file line number Diff line number Diff line change
Expand Up @@ -95,8 +95,8 @@ def writeAvi(
# Get fps
try:
fps = float(1.0 / duration)
except Exception:
raise ValueError(_("Invalid duration parameter for writeAvi."))
except Exception as e:
raise ValueError(_("Invalid duration parameter for writeAvi.")) from e

# Determine temp dir and create images
tempDir = os.path.join(os.path.expanduser("~"), ".tempIms")
Expand Down
8 changes: 4 additions & 4 deletions python/grass/pydispatch/dispatcher.py
Original file line number Diff line number Diff line change
Expand Up @@ -213,18 +213,18 @@ def disconnect(receiver, signal=Any, sender=Any, weak=True):
try:
signals = connections[senderkey]
receivers = signals[signal]
except KeyError:
except KeyError as e:
raise errors.DispatcherKeyError(
"""No receivers found for signal %r from sender %r""" % (signal, sender)
)
) from e
try:
# also removes from receivers
_removeOldBackRefs(senderkey, signal, receiver, receivers)
except ValueError:
except ValueError as e:
raise errors.DispatcherKeyError(
"""No connection to receiver %s for signal %s from sender %s"""
% (receiver, signal, sender)
)
) from e
_cleanupConnections(senderkey, signal)


Expand Down
4 changes: 2 additions & 2 deletions python/grass/pygrass/raster/category.py
Original file line number Diff line number Diff line change
Expand Up @@ -110,8 +110,8 @@ def _chk_index(self, index):
if isinstance(index, str):
try:
index = self.labels().index(index)
except ValueError:
raise KeyError(index)
except ValueError as error:
raise KeyError(index) from error
return index

def _chk_value(self, value):
Expand Down
4 changes: 3 additions & 1 deletion python/grass/pygrass/rpc/base.py
Original file line number Diff line number Diff line change
Expand Up @@ -185,7 +185,9 @@ def safe_receive(self, message):
except (EOFError, OSError, FatalError) as e:
# The pipe was closed by the checker thread because
# the server process was killed
raise FatalError("Exception raised: " + str(e) + " Message: " + message)
raise FatalError(
"Exception raised: " + str(e) + " Message: " + message
) from e
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
) from e
) from None

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This one I'm not sure we'd want this, because there are multiple types of errors caught. So the traceback will be able to tell us which one is the direct cause of the others


def stop(self):
"""Stop the check thread, the libgis server and close the pipe
Expand Down
6 changes: 3 additions & 3 deletions python/grass/pygrass/vector/table.py
Original file line number Diff line number Diff line change
Expand Up @@ -847,10 +847,10 @@ def connection(self):
return psycopg2.connect(db)
except ImportError:
er = "You need to install psycopg2 to connect with this table."
raise ImportError(er)
raise ImportError(er) from None

str_err = "Driver is not supported yet, pleas use: sqlite or pg"
raise TypeError(str_err)
raise TypeError(str_err) from None

def table(self):
"""Return a Table object.
Expand Down Expand Up @@ -1189,7 +1189,7 @@ def execute(self, sql_code=None, cursor=None, many=False, values=None):
"The SQL statement is not correct:\n%r,\n"
"values: %r,\n"
"SQL error: %s" % (sqlc, values, str(exc))
)
) from exc

def exist(self, cursor=None):
"""Return True if the table already exist in the DB, False otherwise
Expand Down
10 changes: 5 additions & 5 deletions python/grass/script/core.py
Original file line number Diff line number Diff line change
Expand Up @@ -901,17 +901,17 @@ def _parse_opts(lines: list) -> tuple[dict[str, str], dict[str, bool]]:
break
try:
var, val = line.split(b"=", 1)
except ValueError:
except ValueError as err:
msg = "invalid output from g.parser: {}".format(line)
raise SyntaxError(msg)
raise SyntaxError(msg) from err
try:
var = decode(var)
val = decode(val)
except UnicodeError as error:
msg = "invalid output from g.parser ({error}): {line}".format(
error=error, line=line
)
raise SyntaxError(msg)
raise SyntaxError(msg) from error
if var.startswith("flag_"):
flags[var[5:]] = bool(int(val))
elif var.startswith("opt_"):
Expand Down Expand Up @@ -1917,7 +1917,7 @@ def _set_location_description(path, location, text):
else:
fd.write(os.linesep)
except OSError as e:
raise ScriptError(repr(e))
raise ScriptError(repr(e)) from e


def _create_location_xy(database, location):
Expand Down Expand Up @@ -1963,7 +1963,7 @@ def _create_location_xy(database, location):
shutil.copy(default_wind_path, wind_path)
os.chdir(cur_dir)
except OSError as e:
raise ScriptError(repr(e))
raise ScriptError(repr(e)) from e
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This basically looks like a pointless try except, as the original Error is raised, so why using try-except here, if not the scripterror simplifies the error message presented to the user...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ScriptError seems to be the error which is raised by related functions, so I think is is for consistency.

With that said, OSError seems like a fine exception to be raised from project creation functions (which would remove the need for try-except).



# interface to g.version
Expand Down
4 changes: 2 additions & 2 deletions python/grass/script/task.py
Original file line number Diff line number Diff line change
Expand Up @@ -470,7 +470,7 @@ def get_interface_description(cmd):
"Unable to fetch interface description for command '<{cmd}>'."
"\n\nDetails: <{det}>"
).format(cmd=cmd, det=e)
)
) from e
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here is another (of several) examples, where the original Error messages is included in the error message that is raised at the end. So, I guess we also here could replace the formating with exception chaining?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe this one with a from None?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The motivation for these Details: is to give user the original exception. This would be better done with chained exceptions in Python. However, the code was written with the expectation that the error will be read by a command line tool user without a traceback (or the original exception). Using from None preserves that idea, but I would be open to a solution which goes the path with less expectations about the potential usage.

Perhaps from e combined with the Details: has most information when doing print(...) and when just looking at the traceback.


desc = convert_xml_to_utf8(cmdout)
return desc.replace(
Expand Down Expand Up @@ -498,7 +498,7 @@ def parse_interface(name, parser=processTask, blackList=None):
_("Cannot parse interface description of<{name}> module: {error}").format(
name=name, error=error
)
)
) from error
task = parser(tree, blackList=blackList).get_task()
# if name from interface is different than the originally
# provided name, then the provided name is likely a full path needed
Expand Down
Loading
Loading