run_command(error='status') ignored by python scripting lib

[landam]

It seems that

run_command(error='status')

is not working as expected. Discovered (https://github.com/OSGeo/grass/blob/master/scripts/r.import/r.import.py#L183) by running r.import which forces reprojection even input data CRS and location matches.

This PR should fix the issue. Only master seems to be affected.

[metzm]

I can confirm the problem, but I wonder if this PR is masking another bug, maybe introduced with cc6dbeb?

[metzm]

@landam @wenzeslaus Looking at the code again, it seems that cc6dbeb has introduced a bug. 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?

[landam]

See also related https://github.com/OSGeo/grass/blob/master/lib/python/script/core.py#L466

BTW, master has been decided to be 8.0 to my understanding

@wenzeslaus Can you elaborate?

[metzm]

The changes look good to me, but I think they are incomplete with regard to fixing cc6dbeb. It seems that the case for errors == 'exit' must also be moved up, because according to the new documentation errors == 'exit' is independent of the returncode.

[wenzeslaus]

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.

[wenzeslaus]

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.

[wenzeslaus]

I agree with the change. 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".

I'm just asking for additional sentence or two in the documentation of handle_errors() to describe the above and perhaps an extra comment in the code to describe the new structure which is less obvious than the previous one (change from "okay state if - error state ifs" to "any state if - okay state if - error state ifs").

I can see three related todos which are out of scope of this PR, but worth mentioning here: cc6dbeb for write_command(), dedicated documentation for each function which is using handle_errors() to cover specific cases and use cases (possibly disallowing some values of the error parameter), and tests.

[metzm]

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

[landam]

Why does this change imply no other release from master before 8.0? Is this change really important enough to increase the major version?

Agreed, it was not decided than master is automatically '8.x'. There will be probably more 7.x releases, eg. 7.10. Let's create for 8.x a new development branch, like develbranch_8 which can be merged with master later when clearly decided.

[landam]

@wenzeslaus Please suggest the fix in a new PR. The master should be synced with relbr78 behaviour, see #547

[petrasovaa]

Replaced by #1839