8000 Adopt API of `tar_options_set` for `geotargets_options_set` · Issue #34 · ropensci/geotargets · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Adopt API of tar_options_set for geotargets_options_set #34

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.

Sign up for GitHub

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

Jump to bottom
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
@Aariq
Labels
priority: high

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 •
edited
Loading

This would also solve #27 and #41

@Aariq Aariq mentioned this issue Mar 15, 2024
Merged
@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
@brownag
Better tar_format function building following ropensci#34, ropensci#43
948a2e6 
 - substitute in options, read/write functions following ropensci#43
 - add support for reading netCDF via `stars::read_ncdf()` using ropensci#43
 - consolidate option getter usage for ropensci#34
@Aariq
Copy link
Collaborator Author
Aariq commented Mar 19, 2024 •
edited
Loading

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 Aariq mentioned this issue Mar 20, 2024
Merged
@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.

80F2
@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
Assignees

@Aariq Aariq

Labels
priority: high
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@brownag @Aariq
0