Skip to content
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

fix delete_layer default in write_sf.DBIObject #1600

Merged
merged 2 commits into from Feb 18, 2021

Conversation

etiennebr
Copy link
Member

  • The default is to delete a table if it exists (append = FALSE)
  • Using append = TRUE will set delete_layer = FALSE and append to an existing table or create one
  • Using append = TRUE and delete_layer = TRUE will raise an error using delete_layer rather than overwrite

fix #1599
related #1266

@etiennebr
Copy link
Member Author

I'll have a look at the failures later today

@edzer
Copy link
Member

edzer commented Feb 10, 2021

regenerate docs.

@etiennebr
Copy link
Member Author

Looks good now.

@edzer
Copy link
Member

edzer commented Feb 10, 2021

Thanks! Could you please double check this is still consistent with the current documentation?

#' @param append logical; should we append to an existing layer, or replace it?
#' if \code{TRUE} append, if \code{FALSE} replace; default \code{NA} will
#' raise an error if the layer exists. See also next two arguments.
#' @param delete_dsn logical; delete data source \code{dsn} before attempting
#' to write?
#' @param delete_layer logical; delete layer \code{layer} before attempting to
#' write?

@etiennebr
Copy link
Member Author

Good point Edzer. I'll see what I can do to use NA to preserve a consistent behavior, but it might be tricky with the database.

@etiennebr etiennebr force-pushed the fix-db-overwrite branch 2 times, most recently from ef7ad1a to caa3a3f Compare February 11, 2021 13:10
@etiennebr
Copy link
Member Author

I'll need to look at this more closely because I'm not sure what would make the most sense from a user perspective. In the sf1.Rmd vignette, it is mentioned that write_sf overwrites by default (which matches other write_* functions).

sf/vignettes/sf1.Rmd

Lines 477 to 485 in 61efbda

```{r}
st_write(nc, "nc.shp", delete_layer = TRUE)
```
or its quiet alternative that does this by default,
```{r}
write_sf(nc, "nc.shp") # silently overwrites
```

However, what seems to be the agreement in #1266 (comment) is:

this commit renames update into append, deprecates update, raises an error if the layer exists but append has not been specified to either TRUE (do append) or FALSE (overwrite layer) .

I decided (for now) against a separate overwrite argument, as setting append=FALSE overwrites. In addition, we do have the delete_dsn and delete_layer logic of GDAL, which does different things to different drivers, which I think we want to keep.

Also As you mentioned:

sf/R/read.R

Lines 329 to 337 in 61efbda

#' @param append logical; should we append to an existing layer, or replace it?
#' if \code{TRUE} append, if \code{FALSE} replace; default \code{NA} will
#' raise an error if the layer exists. See also next two arguments.
#' @param delete_dsn logical; delete data source \code{dsn} before attempting
#' to write?
#' @param delete_layer logical; delete layer \code{layer} before attempting to
#' write?
#' @param fid_column_name character, name of column with feature IDs; if
#' specified, this column is no longer written as feature attribute.

@etiennebr
Copy link
Member Author

Sorry for the hesitation. I would like your approval on this @edzer.

I went back to original write_*() functions form readr, because if I remember correctly this is what the inspiration was for the behaviour.

append If FALSE, will overwrite existing file. If TRUE, will append to existing file. In both cases, if the file does not exist a new file is created.

The default is FALSE, so it overwrites by default.

If we decide to follow this behaviour for write_sf the strict implementation would be to set delete_layer=!append, but I think it would make sense to expose it in the arguments (as in a603d39). In that case, we could document the parameter as:

#' @param append logical; should we append to an existing layer, or replace it? 
#' if \code{TRUE} append, if \code{FALSE} replace. 
#' The default for \code{st_write} is \code{NA} which raises an error if the layer exists. 
#' The default for \code{write_sf} is \code{FASE}, which overwrites by default.
#' See also next two arguments for more control on overwrite behavior. 

Meanwhile I'll go with this version for the PR so you can merge it, but let me know if you would prefer another behaviour.

- The default is `append = FALSE` and `delete_layer  = TRUE` to overwrite a table
- Using `append = TRUE` will append to an existing table or create one
- Using `append = FALSE` or `delete_layer  = TRUE` will raise an error
- Using `overwrite` rather than `delete_layer` will raise an error with a suggestion for a fix.
- Make documentation consistent with behavior; split `dbWriteTable` functions because they have a slightly different behavior.

fix r-spatial#1599
related r-spatial#1266
@etiennebr
Copy link
Member Author

@edzer, let me know if there's anything missing to merge

@edzer edzer merged commit 3ea04bf into r-spatial:master Feb 18, 2021
@edzer
Copy link
Member

edzer commented Feb 18, 2021

Thanks a lot, Etienne!

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.

write_sf() Ignores delete_layer Argument
2 participants