From 8326c8d555276129f9ee1757891a7f41df99f4a5 Mon Sep 17 00:00:00 2001 From: Daniel Sjoberg Date: Mon, 15 Apr 2024 07:37:32 -0700 Subject: [PATCH] Closes #367 `assert_*()` update cleanup (#438) * Deprecating functions we no longer use. * Update NEWS.md * lintr update * warning update * Update quote.R * Update NEWS.md * Update WORDLIST * Update NEWS.md --- NEWS.md | 10 +++++++++ R/compat_friendly_type.R | 7 +++++++ R/dev_utilities.R | 7 +++++++ R/quote.R | 7 +++++++ R/warnings.R | 15 ++++++-------- R/what.R | 7 +++++++ inst/WORDLIST | 1 + tests/testthat/_snaps/warnings.md | 34 +++++++++++++++++++++++++++++++ tests/testthat/test-warnings.R | 20 ++++++++---------- 9 files changed, 87 insertions(+), 21 deletions(-) create mode 100644 tests/testthat/_snaps/warnings.md diff --git a/NEWS.md b/NEWS.md index 66e00cdb..ead706ef 100644 --- a/NEWS.md +++ b/NEWS.md @@ -4,12 +4,22 @@ ## Updates of Existing Functions +* Error messaging throughout the package have been updated from `rlang::abort()` to `cli::cli_abort()`. As a part of the update, each of the `assert_*()` functions have new arguments `assert_*(message, arg_name, call, class).` (#367) + +* Warning messaging has also been updated to use `{cli}` messaging. + ## Breaking Changes * `renv` and related files have been removed. (#360) * No longer exporting `is_named()` function. (#401) +* As a part of the error messaging update, the following changes were made. + + - The `assert_s3_class(class)` argument has been renamed to `assert_s3_class(cls)`. (#367) + + - Functions `arg_name()`, `enumerate()`, `what_is_it()`, and `friendly_type_of()` have been deprecated and a warning is returned to any developer using these functions. As these are developer functions (as opposed to functions for typical admiral users), we will use a short deprecation cycle. + ## Documentation - The "Release Strategy" vignette was updated with respect to the new branching diff --git a/R/compat_friendly_type.R b/R/compat_friendly_type.R index 01ef4ad4..f84345c1 100644 --- a/R/compat_friendly_type.R +++ b/R/compat_friendly_type.R @@ -17,6 +17,13 @@ #' @keywords dev_utility #' @family dev_utility friendly_type_of <- function(x, value = TRUE, length = FALSE) { # nolint + lifecycle::deprecate_warn( + when = "1.1.0", + what = "admiraldev::friendly_type_of()", + details = "This function was primarily used in error messaging, and can be replaced + with 'cli' functionality: `cli::cli_abort('{.obj_type_friendly {letters}}')`." + ) + if (rlang::is_missing(x)) { return("absent") } diff --git a/R/dev_utilities.R b/R/dev_utilities.R index fb540c79..6f7f31b1 100644 --- a/R/dev_utilities.R +++ b/R/dev_utilities.R @@ -44,6 +44,13 @@ convert_dtm_to_dtc <- function(dtm) { #' #' @export arg_name <- function(expr) { # nolint + lifecycle::deprecate_warn( + when = "1.1.0", + what = "admiraldev::arg_name()", + details = "This function was primarily used in error messaging, and can be + replaced with `assert_*(x, arg_name = rlang::caller_arg(x))`" + ) + if (length(expr) == 1L && is.symbol(expr)) { deparse(expr) } else if (length(expr) == 2L && diff --git a/R/quote.R b/R/quote.R index ae0a5b99..03342350 100644 --- a/R/quote.R +++ b/R/quote.R @@ -20,6 +20,13 @@ #' #' @export enumerate <- function(x, quote_fun = backquote, conjunction = "and") { + lifecycle::deprecate_warn( + when = "1.1.0", + what = "admiraldev::enumerate()", + details = "This function was primarily used in error messaging, and can be + replaced with 'cli' functionality: `cli::cli_abort('{.val {letters[1:3]}}')`" + ) + if (is.null(quote_fun)) { quote_fun <- function(x) x } diff --git a/R/warnings.R b/R/warnings.R index dc0cb069..d0250bc7 100644 --- a/R/warnings.R +++ b/R/warnings.R @@ -24,15 +24,12 @@ #' warn_if_vars_exist(dm, "ARM") warn_if_vars_exist <- function(dataset, vars) { existing_vars <- vars[vars %in% colnames(dataset)] - if (length(existing_vars) == 1L) { - msg <- paste("Variable", backquote(existing_vars), "already exists in the dataset") - warn(msg) - } else if (length(existing_vars) > 1L) { - msg <- paste("Variables", enumerate(existing_vars), "already exist in the dataset") - warn(msg) - } else { - invisible(NULL) + if (length(existing_vars) >= 1L) { + cli::cli_warn("{cli::qty(length(existing_vars))}Variable{?s} {.val {existing_vars}} + already {?exists/exist} in the dataset.") } + + invisible(NULL) } #' Warn If a Vector Contains Unknown Datetime Format @@ -74,7 +71,7 @@ warn_if_invalid_dtc <- function(dtc, is_valid = is_valid_dtc(dtc)) { "Missing parts in the middle must be represented by a dash, e.g., 2003---15.", sep = "\n" ) - warn(paste(main_msg, tbl, info, sep = "\n")) + cli::cli_warn(c(main_msg, "*" = tbl, "*" = info)) } } diff --git a/R/what.R b/R/what.R index 575f6652..92c78068 100644 --- a/R/what.R +++ b/R/what.R @@ -18,6 +18,13 @@ #' what_is_it(1:10) #' what_is_it(mtcars) what_is_it <- function(x) { + lifecycle::deprecate_warn( + when = "1.1.0", + what = "admiraldev::what_is_it()", + details = "This function was primarily used in error messaging, and can be replaced + with 'cli' functionality: `cli::cli_abort('{.obj_type_friendly {letters}}')`." + ) + if (is.null(x)) { "`NULL`" } else if (is.factor(x)) { diff --git a/inst/WORDLIST b/inst/WORDLIST index f045a594..cfaea46f 100644 --- a/inst/WORDLIST +++ b/inst/WORDLIST @@ -52,6 +52,7 @@ admiraltemplate admiralxxx advs anonymized +cli cmd codebase codeowner diff --git a/tests/testthat/_snaps/warnings.md b/tests/testthat/_snaps/warnings.md new file mode 100644 index 00000000..db57d56d --- /dev/null +++ b/tests/testthat/_snaps/warnings.md @@ -0,0 +1,34 @@ +# warn_if_vars_exist Test 1: warning if a variable already exists in the input dataset + + Code + warn_if_vars_exist(dm, "AGE") + Condition + Warning: + Variable "AGE" already exists in the dataset. + +--- + + Code + warn_if_vars_exist(dm, c("AGE", "AGEU", "ARM")) + Condition + Warning: + Variables "AGE", "AGEU", and "ARM" already exist in the dataset. + +--- + + Code + warn_if_vars_exist(dm, c("AAGE", "AGEU", "ARM")) + Condition + Warning: + Variables "AGEU" and "ARM" already exist in the dataset. + +# warn_if_invalid_dtc Test 2: Warning if vector contains unknown datetime format + + Code + warn_if_invalid_dtc(dtc = "20210406T12:30:30") + Condition + Warning: + Dataset contains incorrect datetime format: --DTC may be incorrectly imputed on row(s) + * Row 1 : --DTC = 20210406T12:30:30 + * ISO representations of the form YYYY-MM-DDThh:mm:ss.ddd are expected, e.g., 2003-12-15T13:15:17.123. Missing parts at the end can be omitted. Missing parts in the middle must be represented by a dash, e.g., 2003---15. + diff --git a/tests/testthat/test-warnings.R b/tests/testthat/test-warnings.R index a0288440..204110fb 100644 --- a/tests/testthat/test-warnings.R +++ b/tests/testthat/test-warnings.R @@ -3,17 +3,14 @@ test_that("warn_if_vars_exist Test 1: warning if a variable already exists in the input dataset", { dm <- pharmaversesdtm::dm - expect_warning( - warn_if_vars_exist(dm, "AGE"), - "Variable `AGE` already exists in the dataset" + expect_snapshot( + warn_if_vars_exist(dm, "AGE") ) - expect_warning( - warn_if_vars_exist(dm, c("AGE", "AGEU", "ARM")), - "Variables `AGE`, `AGEU` and `ARM` already exist in the dataset" + expect_snapshot( + warn_if_vars_exist(dm, c("AGE", "AGEU", "ARM")) ) - expect_warning( - warn_if_vars_exist(dm, c("AAGE", "AGEU", "ARM")), - "Variables `AGEU` and `ARM` already exist in the dataset" + expect_snapshot( + warn_if_vars_exist(dm, c("AAGE", "AGEU", "ARM")) ) expect_warning( warn_if_vars_exist(dm, "AAGE"), @@ -24,9 +21,8 @@ test_that("warn_if_vars_exist Test 1: warning if a variable already exists in th # warn_if_invalid_dtc ---- ## Test 2: Warning if vector contains unknown datetime format ---- test_that("warn_if_invalid_dtc Test 2: Warning if vector contains unknown datetime format", { - expect_warning( - warn_if_invalid_dtc(dtc = "20210406T12:30:30"), - "Dataset contains incorrect datetime format:" + expect_snapshot( + warn_if_invalid_dtc(dtc = "20210406T12:30:30") ) })