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

[echoix]

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:

It also allows disabling automatic exception chaining using the from None idiom:

>>> try:
...    open('database.sqlite')
... except OSError:
...     raise RuntimeError from None

Traceback (most recent call last):
  File "<stdin>", line 4, in <module>
RuntimeError

https://docs.python.org/3/tutorial/errors.html#exception-chaining

[echoix]

@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.
My third set of changes are still a work-in-progress, as it's a bit a bigger change to make the (ruff) suggestion work in our context. (Since it removes some derived classe's init, I have to adjust the docstrings around and check that the sphinx-generated output is still fine. For now it isn't to my liking.

So, it'll take a couple days to have some more changes ready to review.

[ninsbl]

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?

[echoix]

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.

[echoix]

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.

[echoix]

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

[ninsbl]

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?!?

[ninsbl]

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

[echoix]

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).

[ninsbl]

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

[echoix]

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.

[ninsbl]

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

[ninsbl]

I think we touch upon some general questions here, where I would love to hear the opinion of e.g. @wenzeslaus or @petrasovaa ...

[ninsbl]

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?

[echoix]

Maybe this one with a from None?

[wenzeslaus]

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.

[ninsbl]

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...

[wenzeslaus]

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).

[ninsbl]

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?

[echoix]

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.

[wenzeslaus]

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.

[ninsbl]

Suggested change

	) from e
	) from None

[echoix]

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

[echoix]

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.

[wenzeslaus]

Raising exceptions versus printing error messages

Printing 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 fatal. User will get the error in command line, GUI, or stderr in a subprocess call in Python. The fatal is a shortcut how to print an error message in a standardized way and end the execution of the process. A command line tool has a pretty good idea when to end the process.

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 grass package were envisioned. Similarly to C, the idea for some of these functions was that they are solely meant for direct support of development of command line tools (I suppose; I was not here for that). Hence, it seemed convenient to prefer calls to fatal over raising exceptions. Given the benefits for writing tools, the practice spread widely at least over grass.script which would prevent its full use as a library, but at the same time, a function was introduced to control the behavior of fatal which now by default calls exit, but can also be set to raise an exception allowing its full use as a library. (We can consider for v9 whether the default should be changed.)

[wenzeslaus]

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).

[echoix]

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.