njtierney / geotargets

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

Moves `terra` to Suggests #35

Closed Aariq closed 3 months ago

Aariq commented 3 months ago

Closes #10. Moves terra to Suggests and moves check for terra to top of tar_*() functions.

Also moves validation of filetype to tar_*() functions. I think this might make error traces more readable to users, but haven't really tested thoroughly

njtierney commented 3 months ago

I think to fully close #10 we should implement this using rlang::check_installed().

Aariq commented 3 months ago

I think we should take advantage of rlang::check_installed("pkg") here to provide a neater user experience

I tried using this instead, but the error message from a targets pipeline was not as nice. The code in tar_terra_*() will never be run in an interactive session, which means the user will never see the benefits of rlang::check_installed() (prompting to install missing package) anyway.

> foo <- function() {
+     if (!requireNamespace("asdf")) {
+         stop("package 'asdf' is required", call. = FALSE)
+     }
+ }
> callr::r(foo)
Error: 
! in callr subprocess.
Caused by error: 
! package 'asdf' is required
ℹ See `$stderr` for standard error.
Type .Last.error to see the more details.

> foo2 <- function() {
+     rlang::check_installed("asdf")
+ }
> callr::r(foo2)
Error: 
! in callr subprocess.
Caused by error in `(function () …`:
! The package "asdf" is required.
Type .Last.error to see the more details.
njtierney commented 3 months ago

Great reprex! I'd be tempted to submit that as a bug to rlang - seems like unexpected behaviour, it doesn't even pass the "reason" argument through when specifying. Good catch.

I was thinking that it might be worthwhile passing through a function to manage this (see foo3 that I wrote) - just wrapping the code you wrote. What do you think? It's a small thing, but we use it a couple of times and I think it will also make testing that component of the package easier. Let me know your thoughts :)

foo <- function() {
       if (!requireNamespace("asdf")) {
           stop("package 'asdf' is required", call. = FALSE)
       }
  }
callr::r(foo)
#> Error: ! in callr subprocess.
#> Caused by error: 
#> ! package 'asdf' is required

foo2 <- function() {
     rlang::check_installed("asdf",
                            reason = "Please install asdf")
 }
callr::r(foo2)
#> Error: ! in callr subprocess.
#> Caused by error in `(function () …`:
#> ! The package "asdf" is required Please install asdf

foo3 <- function() {
  .check_pkg_installed <- function(pkg){
    if (!requireNamespace(pkg)) {
      cli::cli_abort("package {.pkg {pkg}} is required",
                     call = rlang::caller_env())
    }
  }

  .check_pkg_installed("asdf")
}

callr::r(foo3)
#> Error: ! in callr subprocess.
#> Caused by error in `(function () …`:
#> ! package asdf is required
callr::r(foo)
#> Error: ! in callr subprocess.
#> Caused by error: 
#> ! package 'asdf' is required

Created on 2024-03-20 with reprex v2.1.0

Session info ``` r sessioninfo::session_info() #> ─ Session info ─────────────────────────────────────────────────────────────── #> setting value #> version R version 4.3.3 (2024-02-29) #> os macOS Sonoma 14.3.1 #> system aarch64, darwin20 #> ui X11 #> language (EN) #> collate en_US.UTF-8 #> ctype en_US.UTF-8 #> tz Australia/Hobart #> date 2024-03-20 #> pandoc 3.1.1 @ /Applications/RStudio.app/Contents/Resources/app/quarto/bin/tools/ (via rmarkdown) #> #> ─ Packages ─────────────────────────────────────────────────────────────────── #> package * version date (UTC) lib source #> callr 3.7.5 2024-02-19 [1] CRAN (R 4.3.1) #> cli 3.6.2 2023-12-11 [1] CRAN (R 4.3.1) #> digest 0.6.34 2024-01-11 [1] CRAN (R 4.3.1) #> evaluate 0.23 2023-11-01 [1] CRAN (R 4.3.1) #> fastmap 1.1.1 2023-02-24 [1] CRAN (R 4.3.0) #> fs 1.6.3 2023-07-20 [1] CRAN (R 4.3.0) #> glue 1.7.0 2024-01-09 [1] CRAN (R 4.3.1) #> htmltools 0.5.7 2023-11-03 [1] CRAN (R 4.3.1) #> knitr 1.45 2023-10-30 [1] CRAN (R 4.3.1) #> lifecycle 1.0.4 2023-11-07 [1] CRAN (R 4.3.1) #> magrittr 2.0.3 2022-03-30 [1] CRAN (R 4.3.0) #> processx 3.8.3 2023-12-10 [1] CRAN (R 4.3.1) #> ps 1.7.6 2024-01-18 [1] CRAN (R 4.3.1) #> purrr 1.0.2 2023-08-10 [1] CRAN (R 4.3.0) #> R.cache 0.16.0 2022-07-21 [2] CRAN (R 4.3.0) #> R.methodsS3 1.8.2 2022-06-13 [2] CRAN (R 4.3.0) #> R.oo 1.26.0 2024-01-24 [2] CRAN (R 4.3.1) #> R.utils 2.12.3 2023-11-18 [2] CRAN (R 4.3.1) #> R6 2.5.1 2021-08-19 [1] CRAN (R 4.3.0) #> reprex 2.1.0 2024-01-11 [2] CRAN (R 4.3.1) #> rlang 1.1.3 2024-01-10 [1] CRAN (R 4.3.1) #> rmarkdown 2.25 2023-09-18 [1] CRAN (R 4.3.1) #> rstudioapi 0.15.0 2023-07-07 [1] CRAN (R 4.3.0) #> sessioninfo 1.2.2 2021-12-06 [2] CRAN (R 4.3.0) #> styler 1.10.2 2023-08-29 [2] CRAN (R 4.3.0) #> vctrs 0.6.5 2023-12-01 [1] CRAN (R 4.3.1) #> withr 3.0.0 2024-01-16 [1] CRAN (R 4.3.1) #> xfun 0.42 2024-02-08 [1] CRAN (R 4.3.1) #> yaml 2.3.8 2023-12-11 [1] CRAN (R 4.3.1) #> #> [1] /Users/nick/Library/R/arm64/4.3/library #> [2] /Library/Frameworks/R.framework/Versions/4.3-arm64/Resources/library #> #> ────────────────────────────────────────────────────────────────────────────── ```
Aariq commented 3 months ago

I was thinking that it might be worthwhile passing through a function to manage this

Yeah! Saves the trouble of remembering to change the package name in two places. Do you want to implement this in this PR?

njtierney commented 3 months ago

Yup, I'll make a couple of changes now.