Fix extraction of ogr LayerName from geodatabase URIs using layerid #3605
Conversation
strk
added
For Review
Requires Tests!
|
@volaya is there a testing framework in use for processing plugin ? |
| @@ -521,6 +523,21 @@ def ogrLayerName(uri): | |||
| regex = re.compile('(layername=)(.*)') | |||
| r = regex.search(uri) | |||
| return r.groups()[1] | |||
| elif '.gdb' in uri: | |||
There was a problem hiding this comment.
why triggering on .gdb only and not layerid= ? Could happen with other multi-layer formats.
There was a problem hiding this comment.
The whole thing looks fragile at the first glace - hosts.shp? dbnames.csv? layernames.dbf?
There was a problem hiding this comment.
It is to handle the case in which there's no "|layerid" nor "|layername".
Can that case be generalized ?
There was a problem hiding this comment.
Actually if you changed your "elif '.gdb' in uri:" as a simple else (and remove the existing else clause), that should be more general
|
@volaya was python/plugins/processing/tests/VectorTest.py a good choice for a unit test for python/plugins/processing/tools/vector.py ? |
strk
removed
the
Requires Tests!
strk
force-pushed
the
release-2_16-fix-geodatabase-import
branch
from
975c32e
to
980b569
Compare
|
Generalized, equipped with tests and squash-rebased |
|
@strk master has https://github.com/qgis/QGIS/blob/master/python/plugins/processing/tests/ToolsTest.py which covers some of the vector stuff - you could safely backport just this file and add your tests there. All the existing tests are just protections while refactoring. |
|
Thanks @nyalldawson, backported the test to 2.16 branch (and noticed there was an API break between 2.16 and 2.18, expected?) |
|
Now I have to fix the expectances of my test: @rouault is lack of "layer*" specification completely undefined as for which file would be considered "layer0" for a directory dataset ? |
Yes, more or less. For a given dataset on a given machine, the order will presumably remain constant if the OS readdir() or equivalent call used returns file names in the same order. |
|
@strk Hmmm... What was the api break? |
|
@nyalldawson features() taking only 1 argument, while in master_2 the two final tests are passing 2 arguments (I've omitted those two in the backport) |
Adds supports for "layerid" when present. Drop special handling for "table=" portions found in URI, making the code more generic. Includes testcase. Fixes qgis#15698 - import geodatabase to postgis via processing
|
btw Nyall, you're an year ahead in the copyright notice :) |
strk
force-pushed
the
release-2_16-fix-geodatabase-import
branch
from
4d3ce1b
to
eca4199
Compare
|
@strk it's not a break though, because the new argument has a default value. So old code will still work. |
|
Pushed as squashed commit 1b1b238 Thanks for confirming, @nyalldawson |
|
@strk test introduced to fix this issue started to fail due different factors:
|
Closes #15698
\cc @volaya, @rouault