njtierney / geotargets

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

Options makeover #45

Closed Aariq closed 2 months ago

Aariq commented 3 months ago

Refactor to address #34. I've taken the approach of not setting any options on package load, but rather storing defaults for individual options in the relevant functions.

This PR does two main things:

  1. Refactors how we create write functions to take advantage of the new tar_resources_custom_format() function in the development version of targets to pass options to write functions in custom formats.
    • This provides a cleaner alternative to eval(substitute()) and works with distributed crew workers.
    • see example
    • Unfortunately, requires targets to be installed from GitHub for now. Eventually will change version requirement to >=1.6.1 and remove the Remotes field from DESCRIPTION.
  2. Simplifies the way geotargets_option_get() and geotargets_option_set() work.
    • geotargets_option_set() is now just a wrapper around options() with some guardrails.
    • geotargets_options_get() looks in getOption() and then in Sys.getenv() for options, otherwise returns NULL. It also checks if the option name is valid with rlang::arg_match0()
    • Defaults options are not set. Defaults are stored in tar_*() functions
Aariq commented 3 months ago

Just realized this is definitely not ideal as any geotargets options set in .Rprofile will be overridden on package load. I like the idea of storing geotargets options in a separate environment that is initialized on package load, I just don't understand how to do it (yet).

njtierney commented 3 months ago

I'm wondering if we could maybe use some of the withr::local_options() type things to test how these options work? The testthat "test figures" vignette provides some nice examples here.

I'm happy for you to merge this currently if you're happy with it and we can add a separate issue about adding more comprehensive tests for setting/getting options? Also happy to start the work on that issue if you'd like a break from it :)

Aariq commented 3 months ago

I'm happy for you to merge this currently if you're happy with it and we can add a separate issue about adding more comprehensive tests for setting/getting options? Also happy to start the work on that issue if you'd like a break from it :)

No, I think I'll add tests before this gets merged. I want to make sure that options set globally actually get used correctly in a targets pipeline.

I also keep going back and forth on whether running geotargets_options_set() with no arguments should do anything (or if we even need that function). Perhaps the logic in geotargets_options_set() (user arg > options > .Renviron) should just be in each tar_*() function?

Aariq commented 3 months ago

In #19 I wondered whether it would be easier to maintain if they were all set in the options get function, but this is probably a better solution: there may be special cases handled in specific tar_*() functions that can't be handled more generally

I keep going back and forth on this. After taking a long break from this PR, I'm now thinking it makes more sense for tar_options_get() to handle the logic of user-supplied %||% getOption() %||% env var %||% default (which, I realize, is basically how it works currently).

Also, using options() doesn't work for pipelines with multiple workers 🤦‍♂️.

Going to study how options are done in targets some more and come back to this.

Aariq commented 2 months ago

Edited the PR description up top and now I think this is ready to review

Aariq commented 2 months ago

Tests should start passing again once targets v 1.7.0 hits CRAN