temporal: Unused variables in temporal modules#1082
temporal: Unused variables in temporal modules#1082nobeeakon wants to merge 1 commit intoOSGeo:mainfrom
Conversation
wenzeslaus
left a comment
wenzeslaus
left a comment
There was a problem hiding this comment.
Thank you. This looks great overall. I have some questions and one request for change (see the comments).
Please, try to run all tests in temporal locally to see if it breaks anything. I think it will show that the one for t.rast3d.algebra is failing (see the comment). You can see it failing in Checks > CI > ubuntu-18.04 tests > Run tests, but if you run it locally, you can run it with and without your changes and see the specific errors.
| cls.use_temp_region() | ||
| ret = grass.script.run_command("g.region", n=80.0, s=0.0, e=120.0, | ||
| w=0.0, t=100.0, b=0.0, res=10.0) | ||
|
|
There was a problem hiding this comment.
Did you try to run this test? This call has an "side effect" meaning it does something else than just generate the return value, specifically it sets the computational region to whatever the test is meant to be executed with. The run_command() return value can be ignored in this case (it should be None actually). Removing the variable is the solution, while the call needs to stay in place.
There was a problem hiding this comment.
Great, thanks for the comments, yeah, completely missed the testing. However, I'm not completely sure how to run them locally, if you could give me some light on that I would really appreciate it.
There was a problem hiding this comment.
For running tests you need NC SPM sample dataset. Use the alpha version from http://fatra.cnr.ncsu.edu/data/nc_spm_full_v2alpha2.tar.gz to have the same one used in the CI.
The commands to run the tests are at the following page under Running tests and creating report and under Running individual test files. You can run a single file or a (sub)directory.
https://grass.osgeo.org/grass79/manuals/libpython/gunittest_running_tests.html
| output="dummy", run_=False, | ||
| finish_=False, flags=flags, | ||
| type=method, overwrite=overwrite, | ||
| column=column, |
There was a problem hiding this comment.
Good catch. Do you also consider the previous state a bug?
Mentioning @veroandreo to have this confirmed.
There was a problem hiding this comment.
Well, I'm not sure. This is a call to r.to.vect which default for column parameter is value. Hence dunno if needed here. In the end the module converts each raster from a strds into a vector and registers them as stvds, what other thing would we store when converting a raster to a vector if not the value? All the examples in r.to.vect omit the column parameter
There was a problem hiding this comment.
In both cases, the documentation says it is Name of attribute column to store value. So according to the doc, it is a name, not what should be be stored. (In theory, you could store, e.g., color, but that's not the case here.) Then it makes sense to pass it and since there is a default which is often good enough, it is not surprising that nobody complained.
There was a problem hiding this comment.
Yes, it puts value as default column name. Since there's no option to store any other attribute, I believe it's a reasonable default name. Of course, maybe better to have it explicitly there in case eventually r.to.vect allows to store something else and hence the name value would no longer apply
|
|
||
| D = tgis.open_old_stds("R", type="strds") | ||
|
|
||
| maplist = D.get_registered_maps_as_objects() |
There was a problem hiding this comment.
Similarly to the ret = grass.script.run_command line, these also do something, but since the test is about something else there seems to be no reason to just test if get_registered_maps_as_objects() executes, nor further investigate its result. So, I agree with removing those.
|
Bumping up milestone as 8.0.1 is due in two days, while this has not been part of RC1 and there has not been activity for some time. |
|
Some conflicts need to be solved manually, they are not editable in the UI in this case. |
echoix
added
conflicts/needs rebase
|
@arohanajit Can you look if any of the changes are still relevant (in other words, have they all been already addressed?) If it's not needed anymore, we'll close the PR. |
Just checking in on the current state of the repo, the issues addressed in this PR are not present anymore. These files still have |
|
So you suggest closing this ? |
|
Yes, I don't think this is relevant anymore |
7 participants
Remove unused variables from temporal module.
removed F841, # local variable... from temporal/.flake8