OSGeo / grass

GRASS GIS - free and open-source geospatial processing engine
https://grass.osgeo.org
Other
849 stars 309 forks source link

[Bug] Unused variables in temporal modules #575

Open wenzeslaus opened 4 years ago

wenzeslaus commented 4 years ago

Describe the bug There are unused variables in three temporal (t.) modules and some tests. It is not immediately clear if these can be just removed or they are actually supposed to be used and the code need to be fixed. Note that one is module option.

To Reproduce

Go to the temporal directory in GRASS GIS source code:

cd temporal

(Install Flake8 and) Run:

flake8 --show-source -j4 --select=F841 --isolated .

Expected behavior No Flake8 error Local variable name is assigned to but never used (F841). Variables should be either used or not present.

Flake8 Errors

./t.rast.algebra/testsuite/test_raster_algebra.py:395:9: F841 local variable 'maplist' is assigned to but never used
        maplist = D.get_registered_maps_as_objects()
        ^
./t.rast.algebra/testsuite/test_raster_algebra.py:412:9: F841 local variable 'maplist' is assigned to but never used
        maplist = D.get_registered_maps_as_objects()
        ^
./t.rast.algebra/testsuite/test_raster_algebra.py:429:9: F841 local variable 'maplist' is assigned to but never used
        maplist = D.get_registered_maps_as_objects()
        ^
./t.rast.algebra/testsuite/test_raster_algebra.py:446:9: F841 local variable 'maplist' is assigned to but never used
        maplist = D.get_registered_maps_as_objects()
        ^
./t.vect.algebra/t.vect.algebra.py:65:5: F841 local variable 'stdstype' is assigned to but never used
    stdstype = "stvds"
    ^
./t.rast.what/t.rast.what.py:386:9: F841 local variable 'map_list' is assigned to but never used
        map_list = output_time_list[count]
        ^
./t.rast.to.vect/t.rast.to.vect.py:138:5: F841 local variable 'column' is assigned to but never used
    column = options["column"]
    ^
./t.rast3d.algebra/testsuite/test_raster3d_algebra.py:28:9: F841 local variable 'ret' is assigned to but never used
        ret = grass.script.run_command("g.region", n=80.0, s=0.0, e=120.0,
        ^

Additional context The code for each module, documentation for the library functions used, and module documentation should provide a clear idea about which variables are supposed to be used and which can be safely removed.

Additionally, the fix can also remove the line F841, # local variable... from the Flake8 configuration file for the temporal directory (temporal/.flake8).

nobeeakon commented 4 years ago

Hi there, I'd like to give it a try. I'm a bit new with coding but I'd really like to contribute.

wenzeslaus commented 4 years ago

Hi and welcome! That sounds great. If you have some updates or questions, you can post them here. Perhaps you are already there, but you can also sign up for the grass-dev mailing list to discuss some more general questions. Even fixing one or two of those would be quite helpful!

nobeeakon commented 4 years ago

Thanks for the opportunity. I'm on it.

nobeeakon commented 4 years ago

Hi again, I think I've fixed the issues. I've also removed the "F841, # local variable.." line form temporal/.flake8 and modified temporal/t.vect.algebra/t.vect.algebra.html since it had a paragraph referring to raster datasets.

I've forked GRASS repository, and don't know how to proceed.

wenzeslaus commented 4 years ago

@nobeeakon If you have the fork, you need to clone it, create a local branch, push this branch to your fork, and then GitHub will show you here and there buttons to open a pull request (PR). Here is the contributing guide to follow which discusses this in detail:

https://github.com/OSGeo/grass/blob/master/CONTRIBUTING.md

Most of the general instructions you can find online will apply here as well. If something is not right, we can address that even after you open the PR.

The documentation change does not seem related, so it would be better to have it in separate branch and create a different PR for that.

Let me know if you hit any roadblocks.

nobeeakon commented 4 years ago

@wenzeslaus done. I finally created the PR for this issue. Long time but finally done...

1082 for the unused variables

and

1089 for the doc temporal/t.vect.algebra/t.vect.algebra.html