[GRASS-dev] PyGRASS Module error handling

[Pietro]

Hi folks,

sorry for the silence...

On Tue, Jul 1, 2014 at 2:33 AM, Sören Gebbert
<soerengebbert at googlemail.com> wrote:
> 2014-06-30 17:43 GMT+02:00 Vaclav Petras <wenzeslaus at gmail.com>:
>>>>> # running for the second time
>>>>> m.run()
>> Traceback (most recent call last)
>>     ...
>> AttributeError: 'str' object has no attribute 'fileno'
>
> That should be fixed in r61083. Reason was the wrong handling of
> stderr and stdout ,
> the PIPE was overwritten when the module finished.


Thanks for the fix!


>> Second, when you confuse m.inputs['map'].value with m.inputs['map'] (which
>> is pretty easy error to do) you get "TypeError: deletions are implemented
>> differently for unicode":
>>
>>>>> # this is OK
>>>>> m.inputs['map'].value = 5
>> Traceback (most recent call last)
>>     ...
>> TypeError: The Parameter <map>, require: raster, get: <type 'int'> instead
>>>>> # this is not OK
>>>>> # confusing parameter object and parameter value
>>>>> m.inputs['map'] = 'aspect'
>> Traceback (most recent call last)
>>     ...
>> TypeError: deletions are implemented differently for unicode

Sorry for the misleading error. I try to explain a bit better what is
going on...

IMHO the best way to set/change the parameter value is:

>>> m.inputs.map
'slope'
>>> m.inputs.map = 'aspect'

Using the syntax:

>>> m.inputs['map'].value

We are asking to a dictionary the object Parameter labelled 'map' and
therefore access to the attribute value of Parameter class.

When you are confusing the parameter object with the value:

>>> m.inputs['map'] = 'aspect'

you are trying to overwrite the instance Parameter with a unicode
string, and there was an error, now should be clear (r61091):

>>> from grass.pygrass.modules import Module
>>> m = Module("r.slope.aspect")
>>> m.inputs['elevation'] = "elevation"
Traceback (most recent call last):
  File "<ipython-input-4-047eba12e7ac>", line 1, in <module>
    m.inputs['elevation'] = "elevation"
  File "/home/pietro/docdat/src/gis/grass71/dist.x86_64-unknown-linux-gnu/etc/python/grass/pygrass/modules/interface/typedict.py",
line 40, in __setitem__
    raise TypeError(str_err % (value, self._type.__name__))
TypeError: The value: 'elevation' is not a Parameter instance.


> This is indeed a misleading error report. I do not have a solution right away,
> but i would suggest to use always the __call__ function when
> specifying an option:
>
> color = Module("r.colors", run_=False)
> color(map="test")
> color(color="gyr")
> color.run()
> color(map="test2", color="ryb")
> color.run()
>
>> With the former it depends what is actually allowed. Than it is probably
>> easy to fix. With the later, no easy solutions come to my mind.
>>
>> With my tests I was actually asserting m.outputs['stdout'] for true which is
>> always true instead of asserting m.inputs['stdout'].value. I don't know how
>> to avoid this mistake in general.

yes it is annoying! but using this syntax you are accessing at the
Parameter instance, this is why I add the other syntax.

IMHO are both useful, when I'm writing a GRASS script I prefer to
access through m.inputs.parameter_name instead when I writing code
that have to handle a Module instance it is useful to be able to
access as a dictionary with: m.inputs['parameter_name'].value

this is the best that I was able to do, to make it smoother and
easier, but If you have any better Idea, please tell me, I will happy
to change the current approach.

> We should avoid in general to set the option values directly, instead
> the __call__ function should be used
> which is much safer.

Using the __call__ method these operations are done internally and you
are pretty sure to not confuse and/or overwrite the Parameter
instance.


>> Does this make sense to you or do you think that I and PyGRASS users should
>> be just be more careful?
>
> The error report must be modified to be more meaningful. I would
> suggest to overwrite the assignment function "=" (if this is
> possible???) of the Parameter class to catch such errors. The
> assignment method will work in case of a Parameter object and raise a
> meaningful error in case anything else. Does this sounds reasonable?

Hi hope that the message error now is clearer, let me know if you
think that still need to be changed!

Pietro