[Bug] r.fillnulls tests no nulls case by using error message text

[wenzeslaus]

Describe the bug

r.fillnulls uses text of an error message in English to test if the process failed because of no null cells in the input. The condition looks like this:

if "No NULL cells found" in stderr:

To Reproduce

Look into the source code, but I assume running with a language other than English would show that it behaves differently in that case.

Expected behavior

Simple fix would be just to rely on the underlying message to avoid the strange code, but a better fix would provide the r.fillnulls specific message using some condition or even analysis (such as r.univar).

[HuidaeCho]

I think a non-English language should work fine thanks to https://github.com/OSGeo/grass/blob/113e0d9c7f4967d5529e0dc824103a13aeb80230/scripts/r.fillnulls/r.fillnulls.py#L585 Still, I agree with you. Relying on output messages from another script looks very fragile. Shouldn't we ideally check the exit code EXIT_FAILURE from G_fatal_error() to avoid running another module?

[wenzeslaus]

I think a non-English language should work fine...

I missed the new_env["LC_ALL"] = "C". The simple fix would be then just a comment.

Relying on output messages from another script looks very fragile.

Right. Very fragile.

Shouldn't we ideally check the exit code EXIT_FAILURE from G_fatal_error() to avoid running another module?

It depends on what is the behavior of r.resamp.bspline. It seems that the return code is zero even if No NULL cells found given that CalledModuleError is not raised based on the r.fillnulls code. ...Wait, no, CalledModuleError is never raised from start_command. The only thing which can raise it there is run_command for g.copy. Maybe what is needed is an addition to r.resamp.bspline so it copies the raster when there are no null cells and gives a warning. Well, unless that's how it already behaves, but it doesn't seems so given that the stderr check was introduced in 542180029569c865cc26cf1b901b7146d80aeb0f (@marisn 2018).

[marisn]

Maybe what is needed is an addition to r.resamp.bspline so it copies the raster when there are no null cells and gives a warning.

This could be an option. Keep in mind – any approach of detecting presence or absence of NULL cells in advance require full scan of the input map as there might be no NULLs inside of computational region. I am running r.fillnulls in tasks taking weeks on a small cluster thus any speed penality results in extra days of waiting :-( It doesn't mean that I do not look back with a horror on the code I wrote some years a go. Yes, r.fillnulls would benefit from some love.

r.mapcalc expression='plane=1'
r.resamp.bspline -n input=plane output=filled  
echo $?  
1