search

osmcode / osmcoastline

Extracts coastline data from OpenStreetMap planet file.
https://osmcode.org/osmcoastline/
GNU General Public License v3.0
109 stars 14 forks source link

Add "ESRI Shapefile driver" as a command line option in addition to "SQLite driver" #36

Closed liyinxiao closed 4 years ago

liyinxiao commented 4 years ago

This pull request is related to this issue.

Test plan: ./osmcoastline -o test.db -v ~/Downloads/hawaii-latest.osm.pbf ./osmcoastline -o test_shapefile -v -g "ESRI Shapefile" ~/Downloads/hawaii-latest.osm.pbf ./osmcoastline -o test.db -v ~/Downloads/rhode-island-latest.osm.pbf ./osmcoastline -o test_shapefile -v -g "ESRI Shapefile" ~/Downloads/rhode-island-latest.osm.pbf All commands run as expected.

My opinion on "supporting all GDAL drivers" I have tested a few GDAL drivers such as GEOJSON, OpenFileGDB, OSM, and all of them return errors for various reasons. For example, GeoJSON driver doesn't support creating more than one layer, while OSM driver does not support the GDALDriver::Create() operation. As a result, "supporting all GDAL drivers" requires a significant amount of code changes. By adding support only for "ESRI Shapefile", we are adding value to osmcoastline without too much additional work.

joto commented 4 years ago

As mentioned in the issue it doesn't make sense to me to limit the allowed drivers. It is kind of obvious that it doesn't work with something like the OSM driver, but there might be others that work. So I'd be okay with this allowing all drivers and if it doesn't work, it doesn't work and there will be some kind of error. The documentation can say something like "only tested with sqlite and shapefile driver" or so. But that way we are not putting in any "artifical" limits.

Can you also please

  1. investigate why some of the CI checks are failing,
  2. update the man page,
  3. add at least one test with the -g option?
liyinxiao commented 4 years ago

Updates:

joto commented 4 years ago

Thanks. This looks nearly done now. But you forgot to remove the check for the two drivers. And could you at least check in the test script that the shapefile was created?

liyinxiao commented 4 years ago

Updates: