chore: Improve handling of suggested packages, add link in doc

[olivroy]

Addresses #1735, closes #2035.

Briefly, it uses check_installed() in the check_suggested function to make it easier to install DiagrammeR. I guess check_suggests() could be simplified further. (and get the function by chaining rather than naming.

Mind the small tweak in tests to avoid skipping the test. We have to make it believe that we are not testing to avoid skipping the test. and actually record the message.

Note that check_installed() errors in non-interactive sessions.

In an unrelated way, I added a link to the data frame vignette in the dm_nyc13 flights as I had issues to get started and this vignette helped me.

[krlmlr]

Nice! I wonder if we should avoid the loop and go straight to vectors. AFAICT is_installed() and check_installed() are both vectorized and do the right thing. The is_installed() function seems to return a scalar too. We need to be careful with the messages we glue together.

[krlmlr]

On a related note, rlang::check_installed() could be doing what we're doing here -- skip in testing mode and when testthat is installed.

[olivroy]

In tests, I believe that it would only error. You would need skip_if_not_installed(), but from my local tests, skip_if_not_installed() seems to be everywhere it needed to be. And the skipping from the check_suggested function did not seem to be needed.

On a related note, rlang::check_installed() could be doing what we're doing here -- skip in testing mode and when testthat is installed.

Edit: Ok, I see, but this would have to be a FR in rlang.

I think that it could entirely be replaced by check_installed() when the use is required

[krlmlr]

I'd rather remove all skip_if_not_installed() calls from tests, unless the package is something that is needed only for testing. That's where I see the beauty of our check_suggested() implementation -- the checks also (indirectly) check that all routines check for all packages they require.

[krlmlr]

See the battery of the "Without xxx" tests here. We're removing suggested packages, one by one, and check that the tests still pass.

[olivroy]

Maybe something like that could be useful. But I don't know much about chaining.

f <- function(pkg) {
  rlang::local_interactive(FALSE)
  rlang::try_fetch(
    rlang::check_installed("rsvg2"),
    error = function(e) {rlang::inform("The function is enhanced by the following package, ",parent = e, call = rlang::caller_fn())})
  invisible()
}
f()

The function is enhanced by the following package, 
Caused by error in `f()`:
! The package "rsvg2" is required.

[krlmlr]

I don't follow.

Would you like to look into removing the loop from the checking routine? I'm even proposing to have check_installed() call skip(), as you noted, this is a FR for rlang. If not, I'm happy to make this a "standalone", to be added via usethis::use_standalone() -- I'm seeing myself applying this pattern everywhere.

[olivroy]

I could look into it. I may have time next week.

the idea would just be to add an optional parameter

check_installed("DiagrammeR")
#> The package is required.
check_installed("DiagrammeR", optional = TRUE)
#> The package "DiagrammeR" can be used to enhance the function.

But I browsed a little through the use, and most usages in dm seem to be replaceable by check_installed() as the functions require the package to function, and error otherwise.

check_suggest <- function(pkg, call = rlang::caller_env()) {
rlang::local_interactive(FALSE) # To avoid the prompt

rlang::try_fetch(
rlang::check_installed(pkg, reason = "to have an enhanced functionality.", call = call),
error = function(e) {rlang::inform("The function is enhanced by the following package, ", parent = e, call = call)})
invisible()
}
g <- function(x) {
check_suggest(c("ggplot2 (>= 3.5.0)", "rsvg", "sss"))
x**2
}
g(2)
#> The function is enhanced by the following package, 
#> Caused by error in `g()`:
#> ! The packages "ggplot2" (>= 3.5.0) and "sss" are required to have an
#>   enhanced functionality.
#> [1] 4

^{Created on 2023-10-12 with reprex v2.0.2}

The only thing that would need to be changed here is to change required to optional.

[krlmlr]

Thanks!

check_installed() could be improved following what we do here. I propose we straighten our implementation here, make it a "standalone", and then propose an update to rlang. Happy to review more PRs.

[aviator-app]

Current Aviator status

Aviator will automatically update this comment as the status of the PR changes.
Comment /aviator refresh to force Aviator to re-examine your PR (or learn about other /aviator commands).

This PR was merged manually (without Aviator). Merging manually can negatively impact the performance of the queue. Consider using Aviator next time.

---

See the real-time status of this PR on the Aviator webapp.

Use the Aviator Chrome Extension to see the status of your PR within GitHub.