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

Moves terra to Suggests #35

Merged
merged 12 commits into from
Mar 22, 2024
Merged

Moves terra to Suggests #35

merged 12 commits into from
Mar 22, 2024

Conversation

Aariq
Copy link
Collaborator

@Aariq Aariq commented Mar 15, 2024

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

DESCRIPTION Outdated Show resolved Hide resolved
@njtierney
Copy link
Owner

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

R/tar-terra-rast.R Outdated Show resolved Hide resolved
R/tar-terra-vect.R Outdated Show resolved Hide resolved
Copy link
Owner

@njtierney njtierney left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@Aariq
Copy link
Collaborator Author

Aariq commented Mar 18, 2024

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 requiredSee `$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.

Merge commit 'e24a4f1852124e6523bef3d3a152804018e5f7b4'

#Conflicts:
#	R/tar-terra-rast.R
#	R/tar-terra-vect.R
@Aariq Aariq requested a review from njtierney March 19, 2024 18:07
@njtierney
Copy link
Owner

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

Aariq commented Mar 20, 2024

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
Copy link
Owner

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

…ector" or "raster" (currently). This removes a little bit of copied complexity in a few places in the pkg
Copy link
Owner

@njtierney njtierney left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool! I think this does what we wanted :)

I think checks should all pass, I'll leave this up to you to hit the merge button :)

@Aariq Aariq merged commit 3429b79 into master Mar 22, 2024
6 checks passed
@Aariq Aariq deleted the terra-suggests-eric branch March 22, 2024 16:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Move all supported r-spatial packages to Suggests
2 participants