-
-
Notifications
You must be signed in to change notification settings - Fork 307
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
t.register: support mapset name in input file #2863
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for updating the test. Can you please enable the test by moving the file to testsuite directory? (I did that recently in b59a047 for r3.out.vtk.) There it would run in CI and fail it if the test fails. A different question is what the test actually tests automatically, but just testing that the modules don't fail is not bad for starters.
Sure. There are more test files though. And I am not sure if all tests are currently successful. I will have a look at them and evtl. port them to Python... |
Seems totally unrelated, but v.to.lines test failed for macOS, have we seen this before?
|
Mac timeout caused fails do occur, but rarely enough for me to not see any pattern. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like that the similar code is replaced by a function, but I have doubts about when the function actually does and what is considered an error.
python/grass/temporal/register.py
Outdated
element = { | ||
"rast": "cell", | ||
"raster": "cell", | ||
"rast3d": "grid3", | ||
"raster3d": "grid3", | ||
"raster_3d": "grid3", | ||
"vector": "vector", | ||
}[type] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How comes it worked before without this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It worked because
a) the find_file
function replaces "raster" with "cell" (and then gives a message if in verbose mode that the user may not be able to understand, see:
grass/python/grass/script/core.py
Lines 1345 to 1347 in ca3b0a8
if element == "raster" or element == "rast": | |
verbose(_('Element type should be "cell" and not "%s"') % element) | |
element = "cell" |
b) if a 3d map was not found (which I guess never was the case) the function simply assumed it is in the current mapset, which may be true for the tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense, but perhaps best hidden in a library function which actually understands the user-facing types and does not complain about them, but that's for a different PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressed in find_file
In order to disentangle the issues a bit here, I created two different feature branches in my fork from which I can create PRs if the direction to solve this is right.
With those two changes in place, the function added in this PR would be obsolet and we could do:
All would basically return the same id. That would appear to me as the clearest implementation with least duplication / repetition of functionality and without changing current behavior. Any thoughts, should I open PRs for the suggested changes to the two functions in question? |
de6ba10
to
a7c0030
Compare
Should be ready to merge as requested changes are addressed... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot. Addressing the other issues in separate PRs helped quite a bit here. Impressive amount of enabled tests and a test for the new and old functionality.
Thanks @wenzeslaus |
Simple |
* support mapset in file * move some tests to Python * use new build_id_from_search_path * add documentation in the manual
Before this PR, input files for registering maps with fully classified map names like here:
threw an error. Now, maps in the register file can be specified with a fully qualified map name (including the mapset name), which should be even faster than using unqualified map names, as existence check is skipped.