[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:9 wenzeslaus]:

 > That's true but I don't know how to solve the following case.
 >
 > I'm currently testing the testing framework.

 Your testing framework probably shouldn't be using functions designed for
 normal scripts. Conversely (and more importantly), features which are
 necessary or useful for a testing framework shouldn't be imposed upon
 normal scripts.

 Write your own "run_command" function atop grass.script.start_command(),
 and use that.

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

 Python's Popen() is at a lower level than the grass.script functions. The
 latter don't accept an "args" parameter, but use grass.script.make_command
 to generate the argument list from the keyword arguments.

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

 All of the functions (except for exec_command) can be replaced by
 start_command, as all of those functions are implemented using
 start_command. They exist as convenience interfaces for the most common
 cases (e.g. being able to use "data = read_command(...)" rather than
 having to explicitly manage a Popen object).

 The cases of a single pipe to either stdin or stdout are simpler (for the
 caller) than those which require multiple pipes, as they avoid the
 potential for deadlock. This is why Unix' popen() lets you read stdout or
 write stderr but not both.

 These interfaces could perhaps be simplified further by supporting the
 context manager interface, so that scripts could use e.g.
 {{{
     with grass.script.pipe_command(...) as p:
         for line in p.stdout:
             # whatever
 }}}
 This would avoid the need for an explicit check of the exit code (the
 check would be performed in the context manager's `__exit__` method).

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

 The "status" in question is the process' exit status, which doesn't exist
 until the process has terminated. How about check_exit_status()?

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

 Another alternative is the same thing under a different name, e.g.
 .finish(). This would still require modifying existing scripts to use that
 rather than .wait(), but at least it keeps it as a method rather than a
 function.

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