njtierney / geotargets

Targets extensions for geospatial data
https://njtierney.github.io/geotargets/
Other
49 stars 4 forks source link

Adopt API of `tar_options_set` for `geotargets_options_set` #34

Closed Aariq closed 2 months ago

Aariq commented 3 months ago

targets::tar_options_set() just uses named args with NULL as default and it might be nice for users to have a similar experience with geotargets_options_set(). E.g.

geotargets_options_set <- function(
  gdal_raster_driver = NULL,
  gdal_raster_creation_options = NULL,
  gdal_vector_driver = NULL,
  gdal_vector_creation_options = NULL
  ){ ...

And because we are setting these options with defaults on package load, we can do arguments to tar_*() functions similar to targets like so:

tar_terra_rast <- function(name,
                           command,
                           pattern = NULL,
                           filetype = geotargets::geotargets_options_get("gdal_raster_driver"),
                           gdal = geotargets::geotargets_options_get("gdal_raster_creation_options"),
                           ...,
                           tidy_eval = targets::tar_option_get("tidy_eval"),

And remove geotargets_options_get() from the body of these functions.

Aariq commented 3 months ago

This would also solve #27 and #41

brownag commented 3 months ago

I really like this idea, I have already been bitten by typos and mistakes with these. Being able to use autocomplete and documentation of function parameters/usage would make it much clearer.

I also like the idea to get the options in the function argument defaults. This could remove most usages, except in the internal functions we pass to tar_format() (until we figure out the covr body<- issue)

Aariq commented 3 months ago

I also think we might want to switch the precedence of env variables and options() to have geotargets_options_set() override options, and options override env variables since .Renviron is only read in on the start of an R session.

So, something like

geotargets_option_get <- function(gdal_raster_creation_options = NULL) {
    geotargets_gdal_raster_creation_options <- gdal_raster_creation_options %||%
        getOption("geotargets.gdal.raster.creation_options", 
                  default =  Sys.getenv("GEOTARGETS_GDAL_RASTER_CREATION_OPTIONS", 
                                        unset = "ENCODING=UTF-8"))

    ...
}
Aariq commented 3 months ago

I asked for some opinions of how to do this in rOpenSci Slack and got two options that both seem good.

1) from Hao Ye & Jon Harmon: on package load, set default options, but only the ones that haven't been set already (via options() or env vars). E.g.: https://github.com/c4r-io/gdrive-automation/blob/c3ce3e5a9963b36ccc480243e96b5ab65eb12872/R/gdrive.automation-package.R#L12

2) from Gábor Csárdi: don't set any options at load time (neither defaults nor from env vars) and just include the defaults in functions (like in PR #45)

I think either of these is compatible with a options_set function, but I'm beginning to think that when run with defaults (i.e. geotargets_option_set()) it should do nothing rather than adding all the missing options.

Aariq commented 2 months ago

Asked for help and got some feedback from Will here: https://github.com/ropensci/targets/discussions/1263#discussioncomment-9063458