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

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

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

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

 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.

 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.

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