Skip to content

Conversation

@sgillies
Copy link
Member

Resolves #771

@sgillies sgillies added this to the 1.8.14 milestone Aug 28, 2020
@coveralls
Copy link

coveralls commented Aug 28, 2020

Coverage Status

Coverage increased (+0.1%) to 86.009% when pulling 24822a3 on issue771 into a6b9708 on maint-1.8.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.5%) to 85.353% when pulling ce0fd54 on issue771 into a6b9708 on maint-1.8.

@rbuffat
Copy link
Contributor

rbuffat commented Aug 28, 2020

@sgillies
Just FYI / inspiration: In #909 some parts of the dataset creation are modified. (

Fiona/fiona/ogrext.pyx

Lines 1026 to 1064 in 6ee5939

# If file exists & we can open it => test if it is possible to add layer
# if not possible => recreate file
# If file exists & we can not open it => recreate file
# If file does not exists => create file
if CPLCheckForFile(path_c, NULL):
try:
cogr_ds = gdal_open_vector(path_c, 1, [collection.driver], open_kwargs)
except DriverError:
try:
# Existing file could be from a different file format. Let first _remove try to guess the
# correct driver
_remove(path)
except:
# Some drivers cannot be guessed, but files can be removed when a driver is specified
try:
_remove(path, collection.driver)
except:
raise DatasetDeleteError("File '{path}' exists and must be deleted "
"manually.".format(path=path))
cogr_ds = gdal_create(cogr_driver, path_c, create_kwargs)
else:
# check capability of creating a new layer in the existing dataset
capability = check_capability_create_layer(cogr_ds)
if GDAL_VERSION_NUM < 2000000 and collection.driver == "GeoJSON":
# GeoJSON driver tells lies about it's capability
capability = False
if not capability or collection.name is None:
# unable to use existing dataset, recreate it
GDALClose(cogr_ds)
cogr_ds = NULL
try:
_remove(path, collection.driver)
except:
raise DatasetDeleteError("File '{path}' exists and must be deleted "
"manually.".format(path=path))
cogr_ds = gdal_create(cogr_driver, path_c, create_kwargs)
else:
cogr_ds = gdal_create(cogr_driver, path_c, create_kwargs)
(all *_kwargs can be ignored))

Currently, there is a special treatment for the GeoJSON that overwrites local files. Would it make sense to have an overwrite=True/False flag in fiona.open() instead, that depending of the result of CPLCheckForFile either returns an error or overwrites? If you are interested, I could create a new PR with just this part.

@sgillies
Copy link
Member Author

@rbuffat I'm 👎 on an overwrite option. I think the mode argument should cover everything. That said, I think I might be implementing "w" wrong for multi-layer formats. It doesn't truncate to zero (overwrite) in that case, where maybe it should. We could require "r+" to add layers to a multi-layer format?

@sgillies sgillies merged commit 290cdd3 into maint-1.8 Aug 28, 2020
@sgillies sgillies deleted the issue771 branch August 28, 2020 19:17
@rbuffat
Copy link
Contributor

rbuffat commented Aug 28, 2020

It doesn't truncate to zero (overwrite) in that case, where maybe it should

If I understand correctly the issue is that is not possible to differentiate between creating a new layer, or overwriting an existing file. If I understand correctly the OGR model strictly separates read and write mode, thus "r+" might be a bit confusing.

Regarding the overwrite aspect, would you just use the existing behavior or having something to separate "w" for writing if the file or layer does not exist and "w!" or "w+" to overwrite a file or layer if it already exists.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants