-
-
Notifications
You must be signed in to change notification settings - Fork 302
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
style: Fix raise-without-from-inside-except (B904) #4032
base: main
Are you sure you want to change the base?
Conversation
@ninsbl That's it for now, I don't have any more queued changes from the last week/weekends that are ready to be converted to a PR. I have one that needs to be applied after the pydispatch code synced from upstream, and another one that is maybe incomplete (it isn't done by ruff, it is manual from the actual pylint failures as ruff doesn't have that rule implemented yet, and since I never got to the point that the first pylint-related step in CI passes, I don't see if there's more. So, it'll take a couple days to have some more changes ready to review. |
9dd14c6
to
d1f0e0e
Compare
@@ -46,7 +46,7 @@ | |||
'The Temporal Plot Tool needs the "matplotlib" ' | |||
"(python-matplotlib) package to be installed. {0}" | |||
).format(e) | |||
) | |||
) from e |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?!?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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...
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we touch upon some general questions here, where I would love to hear the opinion of e.g. @wenzeslaus or @petrasovaa ...
@@ -502,7 +502,7 @@ def get_interface_description(cmd): | |||
"Unable to fetch interface description for command '<{cmd}>'." | |||
"\n\nDetails: <{det}>" | |||
).format(cmd=cmd, det=e) | |||
) | |||
) from e |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
?
There was a problem hiding this comment.
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.
@@ -1939,7 +1939,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 |
There was a problem hiding this comment.
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...
There was a problem hiding this comment.
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).
@@ -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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
raise GrassError(str_err % self.name) from e | |
raise GrassError(str_err % self.name) from None |
Maybe here, and in similar cases too?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
raise FatalError("Exception raised: " + str(e) + " Message: " + message) | ||
raise FatalError( | ||
"Exception raised: " + str(e) + " Message: " + message | ||
) from e |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
) from e | |
) from None |
There was a problem hiding this comment.
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
If we are to address each of these for the best solution in context, it would make sense to expand the scope of the PR and use exception notes where it makes more sense. |
Raising exceptions versus printing error messagesPrinting error messages is for command line tools. Python functions should generally raise exceptions. The Python code in tools (modules; both in core and addons) should generally print an error message calling Python functions in the library should raise exceptions to give the caller freedom to react to them in whatever way is appropriate. This is what is expected from a Python library as exceptions are the standard way in Python of reporting errors and a single function in a library does not have enough context to do anything more specific. Tracebacks may be perceived by users as errors and are less readable then plain error messages, so it is desirable for the command line tool to catch exceptions and print error messages instead. This may also allow for switch from Python and function oriented wording to one more appropriate for a command line tool. However, not all possible exceptions need to be caught. Unlikely and unexpected exceptions unrelated to the standard execution of the program, e.g., programmer's errors, are best left uncaught so that full information in a debugging-friendly format, i.e., traceback, is displayed. The situation in the GUI is similar, although the dividing line is harder to spot and the details would be for a separate discussion. This library-tool distinction gets complicated with how the library functions in the |
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 |
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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.
This PR I want a little bit more attentive review.
Ruff rule: https://docs.astral.sh/ruff/rules/raise-without-from-inside-except/
The flake8 bugbear README also refers to the exception chaining tutorial here: https://docs.python.org/3/tutorial/errors.html#exception-chaining, quite useful to understand what exception chaining is (and is a very good thing for an application like ours when exceptions are rethrown to have more useful details for a user instead of scaring them off).
I wasn't completely sure to understand the existing patterns of exceptions, especially on how they were used.
For the ImportError one, there is no problem, it's obvious that it's good now.
Places where we wouldn't want exception chaining, and also not having the "During handling of the above exception, another exception occurred:" message when raising another exception in an except, we can use the "from None" idiom, and only the exception raised (inside the except) will be shown. Like in the python docs: