Options makeover

[Aariq]

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

[njtierney]

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 :)

[brownag]

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

I think the package should not set options on load so this is a good decision IMO.

If no options are set by the user, it falls back on defaults stored within each tar_*() function.

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

[Aariq]

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]

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]

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

[Aariq]

Tests should start passing again once targets v 1.7.0 hits CRAN

[njtierney]

This all looks really good to me, well done @Aariq! I will update the changes from #50 in here now, and update the tests etc. But this looks like it simplifies things greatly, very nice.