Update package check functions, add skip_if_not_pkg_installed()

[edelarua]

What changes are proposed in this pull request?

• Simplified code in functions used for package checks.
• Added function skip_if_not_pkg_installed() to skip tests if required packages are not installed.

---

Pre-review Checklist (if item does not apply, mark is as complete)

• All GitHub Action workflows pass with a ✅
• PR branch has pulled the most recent updates from master branch: usethis::pr_merge_main()
• If a bug was fixed, a unit test was added.
• If a standalone script was updated, a comment is added to the script header (changelog) AND the last-updated field has been updated.
• Code coverage is suitable for any new functions/features (generally, 100% coverage for new code): devtools::test_coverage()
• Request a reviewer

Reviewer Checklist (if item does not apply, mark is as complete)

• If a bug was fixed, a unit test was added.
• If a standalone script was updated, a comment is added to the script header (changelog) AND the last-updated field has been updated.
• Run pkgdown::build_site(). Check the R console for errors, and review the rendered website.
• Code coverage is suitable for any new functions/features: devtools::test_coverage()

When the branch is ready to be merged:

• All GitHub Action workflows pass with a ✅
• Approve Pull Request
• Merge the PR. Please use "Squash and merge" or "Rebase and merge".
• Create an issue in any repositories using {standalone} to update the standalone scripts.

[ddsjoberg]

Thank you! This is much simpler! Can we make a few small updates?

1. There is a reference to str_detect(). Can we replace this with the base R function so we don't have a dependency on the stringr standalone file?
2. Can we rename skip_if_not_pkg_installed() to skip_if_pkg_not_installed()?
3. We have message = "Not all required packages are installed". Can we add the package names here? Or was this one of the requests from Michael that the message is not unique to the package names?

[edelarua]

@ddsjoberg

1. There is a reference to str_detect(). Can we replace this with the base R function so we don't have a dependency on the stringr standalone file?

Done!

2. Can we rename skip_if_not_pkg_installed() to skip_if_pkg_not_installed()?

Yes! I've updated the name.

3. We have message = "Not all required packages are installed". Can we add the package names here? Or was this one of the requests from Michael that the message is not unique to the package names?

No, he didn't indicate this in his PRs, so I've gone ahead and updated the skip message.

New output:

> devtools::test_active_file()
[ FAIL 0 | WARN 0 | SKIP 2 | PASS 13 ]

── Skipped tests (2) ──────────────────────────────────────────────────────────────────────────────────────────────────────
• interactive() is TRUE (1): test-standalone-check_pkg_installed.R:43:3
• Required package(s) not installed: 'br000000m', 'notapkg' (1): test-standalone-check_pkg_installed.R:74:3

[edelarua]

On second thought, I think the custom error message will result in this same issue Michael flagged with the test logs: insightsengineering/cardx#306

@ddsjoberg what is your interpretation of this? Should we just use one unified error message with no packages listed?

[edelarua]

I've updated the message again so now only the first missing package is printed in each test. This results in a somewhat smaller test log but still lists at least some of the packages that are missing.

In this case the test log from Michael's issue would look something like this (~33% fewer lines):

• Required package 'aod' is not installed (1):
  test-ard_aod_wald_test.R:1:1
• Required package 'broom' is not installed (20):
  test-ard_categorical_ci.data.frame.R:1:1,
  test-ard_continuous_ci.data.frame.R:2:3,
  test-ard_continuous_ci.data.frame.R:20:3,
  test-ard_continuous_ci.data.frame.R:38:3, test-ard_stats_chisq_test.R:1:1,
  test-ard_stats_fisher_test.R:1:1, test-ard_stats_kruskal_test.R:1:1,
  test-ard_stats_mantelhaen_test.R:1:1, test-ard_stats_mood_test.R:1:1,
  test-ard_stats_oneway_test.R:1:1, test-ard_stats_poisson_test.R:1:1,
  test-ard_stats_prop_test.R:1:1, test-ard_stats_t_test_onesample.R:1:1,
  test-ard_stats_t_test.R:1:1, test-ard_stats_wilcox_test_onesample.R:1:1,
  test-ard_stats_wilcox_test.R:1:1, test-ard_stats_mcnemar_test.R:1:1,
  test-ard_stats_anova.R:1:1, test-ard_proportion_ci.R:1:1, 
  test-proportion_ci.R:1:1
• Required package 'car' is not installed (1): test-ard_car_vif.R:1:1
• Required package 'broom.helpers' is not installed (5):
  test-ard_regression_basic.R:1:1, test-ard_car_anova.R:1:1, 
  test-ard_stats_aov.R:1:1, test-construction_helpers.R:1:1,
  test-ard_regression.R:1:1
• Required package 'effect.size' is not installed (2):
  test-ard_effectsize_cohens_d.R:1:1, test-ard_effectsize_hedges_g.R:1:1
• Required package 'emmeans' is not installed (1):
  test-ard_emmeans_mean_difference.R:1:1
• Required package 'survey' is not installed (14):
  test-ard_survey_svychisq.R:1:1, test-ard_survey_svyranktest.R:1:1,
  test-ard_survey_svyttest.R:1:1, test-ard_attributes.survey.design.R:1:1,
  test-ard_survival_survdiff.R:1:1, test-ard_survival_survfit_diff.R:1:1,
  test-ard_survival_survfit.R:1:1, test-ard_categorical_ci.survey.design.R:1:1,
  test-ard_categorical.survey.design.R:1:1,
  test-ard_continuous_ci.survey.design.R:1:1,
  test-ard_continuous.survey.design.R:1:1,
  test-ard_dichotomous.survey.design.R:1:1,
  test-ard_missing.survey.design.R:1:1, test-ard_total_n.survey.design.R:1:1
• Required package 'smd' is not installed (1): test-ard_smd_smd.R:1:1

[ddsjoberg]

Ah, thanks for looking into the details! Let's take his suggestion to only show the message for the first missing pkg. But what I don't love is that we're parsing the DESCRIPTION file so many times. Would something like this work? (I am not sure we need the break...would the function quit if the skip() is TRUE?)

skip_if_pkg_not_installed <- function(pkgs,
                                      ref = utils::packageName()) {
  df_deps <- get_min_version_required(pkgs, ref = ref)
  
  for (i in seq_along(df_deps$pkg)) {
    if (rlang::is_installed(df_deps$pkg[i])) {
      # skip if any required package is not installed
      testthat::skip(
        message = paste(
          "Required package", shQuote(df_deps$pkg[i], type = "sh"), "is not installed"
        )
      )
      break
    }
  }
  
  invisible()
}

[edelarua]

Ah, thanks for looking into the details! Let's take his suggestion to only show the message for the first missing pkg. But what I don't love is that we're parsing the DESCRIPTION file so many times. Would something like this work? (I am not sure we need the break...would the function quit if the skip() is TRUE?)

skip_if_pkg_not_installed <- function(pkgs,
                                      ref = utils::packageName()) {
  df_deps <- get_min_version_required(pkgs, ref = ref)
  
  for (i in seq_along(df_deps$pkg)) {
    if (rlang::is_installed(df_deps$pkg[i])) {
      # skip if any required package is not installed
      testthat::skip(
        message = paste(
          "Required package", shQuote(df_deps$pkg[i], type = "sh"), "is not installed"
        )
      )
      break
    }
  }
  
  invisible()
}

I've updated to use rlang::is_installed() with the deps rather than is_pkg_installed() so get_min_version_required() is only run once. The function will always quit if the skip() is TRUE so the break isn't needed/there's no way to print multiple skip() messages.

Ready for another review whenever you have time!!

[ddsjoberg]

Thank you for the update!!