Skip to content
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.

Sign up for GitHub

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

Jump to bottom

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

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
+40 −36
Open

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
d1f0e0e
style: Fix raise-without-from-inside-except (B904)
echoix Jul 8, 2024
7f0043d
Update frame.py
echoix Jul 13, 2024
eec6c40
Merge branch 'main' into fix-raise-without-from-inside-except
echoix Aug 1, 2024
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/iscatt/plots.py
View file
Open in desktop
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/timeline/frame.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,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
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,9 +44,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.

Or maybe we'd want to use exception notes: https://docs.python.org/3/tutorial/errors.html#enriching-exceptions-with-notes?

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 python/grass/pygrass/modules/interface/module.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -555,7 +555,7 @@ def __init__(self, cmd, *args, **kargs):
except OSError as e:
print("OSError error({0}): {1}".format(e.errno, e.strerror))
str_err = "Error running: `%s --interface-description`."
raise GrassError(str_err % self.name)
raise GrassError(str_err % self.name) 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
raise GrassError(str_err % self.name) from e
raise GrassError(str_err % self.name) from None

Maybe here, and in similar cases too?

Copy link
Member Author

Choose a reason for hiding this comment

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

The code around here is weird, we shouldn't be using print for this there. (It was out of scope of the PR, so I wasn't changing everything, in order to be reviewable). But None makes sense, as OSError is only to know what that we couldn't find the module (or fails), it's not the real exception.

Copy link
Member

Choose a reason for hiding this comment

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

You can get PermissionError or FileNotFoundError which is likely a relevant info. The from e will preserve that, no? ...if yes, that would be an improvement. Semantic of GrassError is not clear to me and Error running: is more hiding than explaining what happened here. The print seems to assume some usage and tries to add convenience. ...yep, some deeper revision would be nice.

# get the xml of the module
self.xml = get_cmd_xml.communicate()[0]
# transform and parse the xml into an Element class:
Expand Down
4 changes: 2 additions & 2 deletions python/grass/pygrass/raster/category.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -110,8 +110,8 @@ def _chk_index(self, index):
if type(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
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -173,7 +173,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
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -859,10 +859,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
else:
str_err = "Driver is not supported yet, pleas use: sqlite or pg"
raise TypeError(str_err)
raise TypeError(str_err) from None
Comment on lines 860 to +865
Copy link
Member

Choose a reason for hiding this comment

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

Wouldn't the except ImportError: raise ImportError cases be best handled by add_note...raise? I would say yes, except that we need to wait for min Python on main version to be raised to 3.11 (PEP).

Copy link
Member Author

Choose a reason for hiding this comment

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

It's always possible to use the feature if it is available and let it go if not supported. Or do a fallback of some sort, that adds the text to the message instead.


def table(self):
"""Return a Table object.
Expand Down Expand Up @@ -1204,7 +1204,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
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -867,8 +867,8 @@ def _parse_opts(lines):
break
try:
var, val = line.split(b"=", 1)
except ValueError:
raise SyntaxError("invalid output from g.parser: {}".format(line))
except ValueError as err:
raise SyntaxError("invalid output from g.parser: {}".format(line)) from err
echoix marked this conversation as resolved.
Show resolved Hide resolved
try:
var = decode(var)
val = decode(val)
Expand All @@ -877,7 +877,7 @@ def _parse_opts(lines):
"invalid output from g.parser ({error}): {line}".format(
error=error, line=line
)
)
) from error
if var.startswith("flag_"):
flags[var[5:]] = bool(int(val))
elif var.startswith("opt_"):
Expand Down Expand Up @@ -1883,7 +1883,7 @@ def _set_location_description(path, location, text):
fd.write(os.linesep)
fd.close()
except OSError as e:
raise ScriptError(repr(e))
raise ScriptError(repr(e)) from e


def _create_location_xy(database, location):
Expand Down Expand Up @@ -1934,7 +1934,7 @@ def _create_location_xy(database, location):

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
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -501,7 +501,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 @@ -529,7 +529,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
6 changes: 3 additions & 3 deletions python/grass/script/vector.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -423,7 +423,7 @@ def vector_what(
try:
ret = read_command("v.what", env=env, **cmdParams).strip()
except CalledModuleError as e:
raise ScriptError(e.msg)
raise ScriptError(e.msg) from e

data = []
if not ret:
Expand All @@ -449,10 +449,10 @@ def vector_what(

try:
result = json.loads(ret, **kwargs)
except ValueError:
except ValueError as err:
raise ScriptError(
_("v.what output is not valid JSON format:\n {ret}").format(ret=ret)
)
) from err

if multiple:
for vmap in result["Maps"]:
Expand Down
6 changes: 4 additions & 2 deletions python/grass/semantic_label/reader.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ def _read_config(self):
except json.decoder.JSONDecodeError as e:
raise SemanticLabelReaderError(
"Unable to parse '{}': {}".format(json_file, e)
)
) from e

# check if configuration is valid
self._check_config(config)
Expand Down Expand Up @@ -116,7 +116,9 @@ def print_info(self, shortcut=None, band=None, semantic_label=None, extended=Fal
if shortcut and re.match(shortcut, item["shortcut"]) is None:
continue
except re.error as e:
raise SemanticLabelReaderError("Invalid pattern: {}".format(e))
raise SemanticLabelReaderError(
"Invalid pattern: {}".format(e)
) from e

found = True
if band and band not in item["bands"]:
Expand Down
14 changes: 7 additions & 7 deletions python/grass/utils/download.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -107,7 +107,7 @@ def extract_zip(name, directory, tmpdir):
extract_dir=extract_dir, target_dir=directory, files=files
)
except zipfile.BadZipfile as error:
raise DownloadError(_("ZIP file is unreadable: {0}").format(error))
raise DownloadError(_("ZIP file is unreadable: {0}").format(error)) from error


# modified copy from g.extension
Expand Down Expand Up @@ -167,9 +167,9 @@ def download_and_extract(source, reporthook=None):
url=source,
code=err,
),
)
except URLError:
raise DownloadError(url_error_message.format(url=source))
) from err
except URLError as e:
raise DownloadError(url_error_message.format(url=source)) from e
if headers.get("content-type", "") != "application/zip":
raise DownloadError(
_(
Expand All @@ -188,9 +188,9 @@ def download_and_extract(source, reporthook=None):
url=source,
code=err,
),
)
except URLError:
raise DownloadError(url_error_message.format(url=source))
) from err
except URLError as e:
raise DownloadError(url_error_message.format(url=source)) from e
extract_tar(name=archive_name, directory=directory, tmpdir=tmpdir)
else:
# probably programmer error
Expand Down
10 changes: 5 additions & 5 deletions scripts/r.in.wms/wms_cap_parsers.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,17 +41,17 @@ def __init__(self, cap_file):
if is_file:
try:
etree.ElementTree.__init__(self, file=cap_file)
except ParseError:
raise ParseError(_("Unable to parse XML file"))
except ParseError as pe:
raise ParseError(_("Unable to parse XML file")) from pe
except OSError as error:
raise ParseError(
_("Unable to open XML file '%s'.\n%s\n" % (cap_file, error))
)
) from error
else:
try:
etree.ElementTree.__init__(self, element=etree.fromstring(cap_file))
except ParseError:
raise ParseError(_("Unable to parse XML file"))
except ParseError as pe:
raise ParseError(_("Unable to parse XML file")) from pe

if self.getroot() is None:
raise ParseError(_("Root node was not found."))
Expand Down
Loading