[Bug] bugs introduced with cc6dbeb in pythonlib

[metzm]

Describe the bug
The behaviour of handle_errors() in lib/python/script/core.py changed with cc6dbeb for no apparent reason, introducing several bugs.

To Reproduce
See bug fixed by #333.

Expected behavior
A clear and concise description of what has changed and why it was changed. The code and the documentation should match.

Additional context
@wenzeslaus @landam
Regarding the changes introduced with cc6dbeb to handle_errors(), the priority of the errors value is not clear and the code does not conform to the documentation. Possible values are apparently "raise", "fatal", "status", "exit", and "ignore", default is "raise". Those errors values independent of the returncode must come first: status, exit, ignore. After that, if the returncode is not 0, the errors values "raise", "fatal", "ignore" must be handled, "raise" will apparently be used if the errors value is not set. At the end, handle_errors() can return result or None for returncode == 0.

What is the reason for versionchanged:: 8.0? Why would returning None instead of 0 be better in case of returncode == 0?

[wenzeslaus]

Copied over from #333 for context:

I don't understand why run_command() is not always returning the returncode. run_command() actually returns handle_errors(). Maybe run_command() should call handle_errors() and return the returncode?
Can you elaborate?

There was a thorough discussion about the desired behavior of run_command() between 7.0 and 7.2 which is recorded in the Trac Ticket 2326. run_command() returning returncode was, I assume, mainly motivated by providing similar behavior to Bash or by designing as in C.

However, a return value is unexpected to Python programmers as exceptions are the usual error reporting mechanism in Python and thus dangerous because you could easily write code which was not checking anything and the error would appear only later. With interactive use, it is also awkward as you get 0 for every successfully executed line which is strange to Python users (not necessary even aware of process returncodes where 0 -> False is the good case and !0 -> True is the failure) and perhaps even unix users (not expecting outputs when everything is fine). It is also worth noting that before that read_command() did not even report the errors in any way and exceptions seem to be the natural way how to handle errors in that case.

Glynn addressed the different use cases (Python-like, immediate fail okay, error expected, more handling needed) by implementing the universal handle_errors() which is used everywhere to decide what to do with the returncode and result (so perhaps the name should be more along the lines of handle_errors_or_return_result()). However, since 7.0 was released, we did not want to completely break compatibility with user scripts written for 7.0 and perhaps even pre-7.0 which was along for a long time, so we kept the returncode being returned from run_command() when returncode == 0 for cases script would still test the return value in an if statement. When all was good, script would work, and a failure would result in a traceback which would not be as good as handling it as the script author intended, but it would at least report the error in the right place.

The case above, when run_command() was used properly was actually not our big concern because it looked like it was not used properly quite often because of wrong assumptions about its behavior coming either from Python philosophy (expecting exceptions) or from GRASS GIS C API philosophy (expecting G_fatal_error()). So the implementation was tradeoff between breaking existing scripts and catching errors in scripts with, at that time, incorrect error handling.

None of the compatibility concerns should be relevant now as we are switching the major version and the proper on-error behavior was already in place since 7.2 (backported to 7.0), so if script used the defaults and was tested for errors in any way the exception would be generated (which is what happened on couple places in GRASS GIS, if I remember correctly, even after trying to address it beforehand, so it seems that the approach worked). Consequently, run_command() can now behave as planned.

The same applies also to write_command() which I should have done together with run_command() in cc6dbeb. So for now, write_command() lacks the [result=]returncode -> result=None change.

And yes, cc6dbeb assumes master is or will be 8.0, i.e., no other release from master before 8.0.

Bug history investigation: handle_errors() since it was introduced in c33ef44 always returned the returncode if the returncode was zero. This is not correct behavior since the result is ignored. The reason it surfaced only after cc6dbeb is that result was set to returncode for run_command() for the backwards compatibility reasons.

As for errors == 'exit' etc., see doc:

The function returns result if returncode is equal to 0, otherwise it reports errors based on the current settings. ... If kwargs dictionary contains key errors, the value is used to determine the behavior on error.

I agree with the change [in #333]. Besides fixing the issue, I think it is actually implementing the originally intended behavior.

With errors="raise", returning 0 on success and raising exception on error does not make much sense as the 0 is not useful for anything (it was useful to keep old code running). With errors="status", returning returncode on error and returning result, which is None in case of run_command(), on success does not make sense either as it makes processing of the return value potentially awkward (although doable since bool(None) == bool(0)). So, it makes sense that errors="status" behaves little differently in the way that return value is always returncode, i.e., result parameter is ignored with errors="status".

[wenzeslaus]

[Moving from #333:]

cc6dbeb assumes master is or will be 8.0, i.e., no other release from master before 8.0.
Why does this change imply no other release from master before 8.0? Is this change really important enough to increase the major version?
Please reply in issue #339

I think there are other reasons for 7.8 being followed by 8.0. This is just an additional one. However, in this case, it just a formality and it could be introduced even without a major version since all code written after 7.2 which properly handles errors will continue to work. Anyway, the .. versionchanged:: 8.0 piece can be fixed once that decided.

The current state is an inconsistent interface which keeps badly written code working. The new interface (with #333 fix and additional updates) better aligns with what Python users/programmers/script authors expect, so it is helpful for the beginners and people who are using more packages than just GRASS GIS. So, I would say it is at least somewhat important.

Alternative solution is to keep the whatever state of grass.script.*_command() functions and introduce another interface which would have the desired behavior. This would be a third interface for running modules, but despite of how that sounds, it is actually more realistic than it looks because with things around the current state of interfaces in grass.* packages and grass-session/grass_session, we may need to do it anyway to address some of the challenges which GRASS GIS has in the current Python landscape.

[metzm]

It seems reasonable that by default handle_errors() returns None instead of returncode 0, and if returncode is not 0, raise an exception. IMHO, this does not require an increase of the major version. E.g. the change in TGIS introducing band reference was much more substantial, breaking pretty much all t.* modules.

Regarding handle_errors(), it would be great if both the documentation and the code match the intended behaviour.

If you think the current code is badly written, please provide either a PR or suggestions on how to improve.

[petrasovaa]

Fixed by #1839.