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

Adopt API of tar_options_set for geotargets_options_set #34

Closed
Aariq opened this issue Mar 15, 2024 · 5 comments
Closed

Adopt API of tar_options_set for geotargets_options_set #34

Aariq opened this issue Mar 15, 2024 · 5 comments
Assignees

Comments

@Aariq
Copy link
Collaborator

Aariq commented Mar 15, 2024

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
Copy link
Collaborator Author

Aariq commented Mar 15, 2024

This would also solve #27 and #41

@brownag
Copy link
Contributor

brownag commented Mar 16, 2024

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)

brownag added a commit to brownag/geotargets that referenced this issue Mar 19, 2024


 - substitute in options, read/write functions following njtierney#43
 - add support for reading netCDF via `stars::read_ncdf()` using njtierney#43
 - consolidate option getter usage for njtierney#34
@Aariq
Copy link
Collaborator Author

Aariq commented Mar 19, 2024

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
Copy link
Collaborator Author

Aariq commented Mar 22, 2024

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 Options makeover #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 Aariq self-assigned this Apr 3, 2024
@Aariq
Copy link
Collaborator Author

Aariq commented Apr 10, 2024

Asked for help and got some feedback from Will here: ropensci/targets#1263 (comment)

@Aariq Aariq closed this as completed Apr 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants