Opened 3 years ago

Last modified 8 months ago

#3100 new defect

v.vol.rst WARNING in GRASS 7 still says cellout and cellinp rasters

Reported by: rorschach Owned by: grass-dev@…
Priority: normal Milestone: 7.0.7
Component: Default Version: 7.0.4
Keywords: Cc:
CPU: x86-64 Platform: Linux

Description

A very minor issue. GRASS 7 changed the names of cellinp and cellout parameters of the v.vol.rst module to cross_input and cross_output respectively but when the module issues a warning it still says:

WARNING: Unable to create cellout raster map without cellinp (emphasis supplied)

This may be minor but I think it throws off a user because there is no cellout or cellinp option anymore in GRASS 7.

I haven't checked if the same goes for other WARNING/ERROR messages for other modules. The main source code probably needs a little proofreading to avoid simple defects like this. I would volunteer but I don't have access to the main svn (yet).

Thank you.

Attachments (1)

v_vol_rst_exclusive_options.diff (2.0 KB) - added by mlennert 3 years ago.
diff to implement options dependency checks through parser

Download all attachments as: .zip

Change History (10)

comment:1 in reply to:  description Changed 3 years ago by hellik

Replying to rorschach:

I haven't checked if the same goes for other WARNING/ERROR messages for other modules. The main source code probably needs a little proofreading to avoid simple defects like this. I would volunteer but I don't have access to the main svn (yet).

you can have a look online at the code of trunk and all the branches here:

https://trac.osgeo.org/grass/browser/grass

or do a svn checkout, see svn checkout

comment:2 Changed 3 years ago by rorschach

Checked the source, found the problem on line 640 of the module's main.c file which reads:

G_warning(_("Unable to create cellout raster map without cellinp"));

Can someone change this to:

G_warning(_("Unable to create cross_output raster map without cross_input"));

for consistency?

Thanks.

comment:3 Changed 3 years ago by wenzeslaus

Thanks. A better fix is using the names as defined in the Option structure by attribute key (example from r.slope.aspect:

G_fatal_error(_("%s=%s - must be a positive number"), parm.zfactor->key, parm.zfactor->answer);

Can you please also check if there are other places like this, e.g. using grep cell and create a patch using svn diff?

comment:4 Changed 3 years ago by neteler

Attempt done in r69439: if ok to be backported

comment:5 Changed 3 years ago by neteler

Milestone: 7.0.57.0.6

comment:6 in reply to:  4 ; Changed 3 years ago by mlennert

Replying to neteler:

Attempt done in r69439: if ok to be backported

Actually, the test tests for the existance of both maps:

if ((cellinp != NULL) && (cellout != NULL)) {

so the message will pop up if either of them is missing or if both are missing, so this is not very helpful. Especially when you don't give either of them, you get:

ATTENTION: Unable to create <(null)> raster map without cross_input raster
           map being specified

I think this should be handled by parameter dependency rules for the parser, and not by the code. Something like:

G_option_collective(parm.cellinp, parm.cellout)

Changed 3 years ago by mlennert

diff to implement options dependency checks through parser

comment:7 in reply to:  6 Changed 3 years ago by mlennert

Replying to mlennert:

Replying to neteler:

Attempt done in r69439: if ok to be backported

Actually, the test tests for the existance of both maps:

if ((cellinp != NULL) && (cellout != NULL)) {

so the message will pop up if either of them is missing or if both are missing, so this is not very helpful. Especially when you don't give either of them, you get:

ATTENTION: Unable to create <(null)> raster map without cross_input raster
           map being specified

I think this should be handled by parameter dependency rules for the parser, and not by the code. Something like:

G_option_collective(parm.cellinp, parm.cellout)

Try the attached patch which implements the above and other checks that were done in the code.

comment:8 Changed 22 months ago by neteler

Milestone: 7.0.67.0.7

comment:9 Changed 8 months ago by martinl

Still relevant?

Note: See TracTickets for help on using tickets.