[GRASS-dev] [GRASS GIS] #2326: Command functions in grass.script.core miss a correct error reporting

[GRASS GIS]

#2326: Command functions in grass.script.core miss a correct error reporting
--------------------------------+-------------------------------------------
 Reporter:  wenzeslaus          |       Owner:  grass-dev@…              
     Type:  enhancement         |      Status:  new                      
 Priority:  major               |   Milestone:  7.1.0                    
Component:  Python              |     Version:  svn-trunk                
 Keywords:  script, exceptions  |    Platform:  All                      
      Cpu:  Unspecified         |  
--------------------------------+-------------------------------------------

Comment(by wenzeslaus):

 Replying to [comment:8 glynn]:
 > Replying to [comment:6 wenzeslaus]:
 >
 > > Here are my suggestions for changes in `grass.script.core`.
 >
 > These changes are excessive. All that is required is to check the exit
 code and generate an error if it is non-zero. If the child process returns
 a zero exit code, the existing behaviour should not be affected.
 >
 > In particular, stderr should not be captured. It isn't limited to errors
 (e.g. messages and percentages are written to stderr), and such
 information should normally be sent to the terminal as its generated.
 >

 That's true but I don't know how to solve the following case.

 I'm currently testing the testing framework. When I make a mistake in my
 testing code, an exception (`ValueError` in this case) is raised:

 {{{
 > python -m unittest discover sandbox/wenzeslaus/gunittest
 ..DBMI-SQLite driver error:
 Error in sqlite3_prepare():
 SELECT cat, YEAR_BUILTX FROM bridges
 no such column: YEAR_BUILTX

 DBMI-SQLite driver error:
 Error in sqlite3_prepare():
 SELECT cat, YEAR_BUILTX FROM bridges
 no such column: YEAR_BUILTX

 ERROR: Column type not supported
 E............................
 ======================================================================
 ERROR: test_assertVectorFitsUnivar
 (testsuite.test_assertions.TestRasterMapAssertations)
 ----------------------------------------------------------------------
 Traceback (most recent call last):
   File
 "/home/vasek/dev/grass/sandbox/wenzeslaus/gunittest/testsuite/test_assertions.py",
 line 80, in test_assertVectorFitsUnivar
     reference=V_UNIVAR_BRIDGES_WIDTH_SUBSET)
   File "/usr/lib/python2.7/unittest/case.py", line 475, in assertRaises
     callableObj(*args, **kwargs)
   File "/home/vasek/dev/grass/sandbox/wenzeslaus/gunittest/case.py", line
 102, in assertVectorFitsUnivar
     reference=reference, msg=msg, sep='=')
   File "/home/vasek/dev/grass/sandbox/wenzeslaus/gunittest/case.py", line
 66, in assertCommandKeyValue
     ": %s\n" % (module, ", ".join(missing)))
 ValueError: v.univar output does not contain the following keys provided
 in reference: max, mean, min, n, nmissing, nnull, range, sum


 ----------------------------------------------------------------------
 Ran 31 tests in 1.227s

 FAILED (errors=1)
 }}}

 But of course the error report is misleading because the problem is not in
 the "key provided" or "v.univar output", the problem is that I used
 current version of `read_command` which does not raise. If the
 `read_command` function raise, I would get `ScriptError` (or whatever)
 with an error message saying that `v.univar` failed. But this is not
 enough to fix the problem.

 If we catch the stderr we can report what has happened. In this case I
 would get a message about wrong column name. However, if we will let
 stderr be printed to the console, we will have to search in the output for
 the errors which does not contain any information about command which
 caused them (unfortunately, in this case they are even wrong and not in
 GRASS format).

 The only option I see is to have both functions. One capturing the stderr
 for cases when the module (command) is used more like a function and one
 letting the stderr go wherever it is directed. But it don't like it
 because this applies at least to three functions which would create 6
 different functions. Parameter, as noted bellow, might be a more
 acceptable solution.

 > Also, checking kwargs!["args"] isn't useful.

 If you mean `cmd = kwargs.get("args")`, this is from `Popen`, source code,
 I don't know what exactly they are trying to accomplish.
 >
 > In most cases, the failure to check exit codes was inherited from the
 original shell script. run_command() replaces "simple" command execution,
 read_command() replaces backticks. pipe_command() and feed_command() are
 used for a GRASS command at one end of a pipeline. write_command()
 replaces "echo data | command".
 >
 Beginning and end of the pipe line still does not convince me about
 usefulness of the functions. I still see them as unnecessary complication
 of interface. If one need something like this, he or she can use
 `start_commnad` directly. Direct working with `Popen` object cannot be
 avoided in any case.

 > My suggestion would be that the functions which wait for the process to
 terminate (run_command, read_command, write_command) should call a
 check_status() function, e.g.
 > {{{
 > def check_status(p, args, kwargs):
 >     if p.wait() == 0:
 >         return 0
 >     raise CalledCommandError(p.returncode, args, kwargs)
 > }}}
 >
 > run_command() and write_command() would replace
 > {{{
 >     return p.wait()
 > }}}
 > with
 > {{{
 >     return check_status(p)
 > }}}
 >
 I don't agree with the name. It does not `check_status` it waits and then
 checks status, so I would suggest `_wait_check_status`.

 > This usage pattern would allow check_status() to be modified to provide
 more options regarding error handling, e.g. raise an exception, call
 fatal(), or just return the exit code, with the behaviour controlled by a
 variable or a keyword argument.
 >
 Sure, this is the way to go (`called_command_error` from comment:7 is
 doing something like that).

 > Alternatively, we could just modify the Popen wrapper to provide a
 modified .wait() method which does this automatically. This would probably
 "do the right thing" for scripts which use start_command() etc and
 subsequently call p.wait() themselves.

 I vote against. The lower level API (when using `Popen` object) should
 behave in the same way as Python `Popen` to allow users/programmers switch
 between them easily. Moreover, it would be a violation of some OOP
 principles, although in case of Python not so big violation, I believe.

-- 
Ticket URL: <http://trac.osgeo.org/grass/ticket/2326#comment:9>
GRASS GIS <http://grass.osgeo.org>