From 43f56a787bba5f6ac09f669035380ad91330c98a Mon Sep 17 00:00:00 2001 From: olivroy Date: Sun, 10 Sep 2023 09:54:48 -0400 Subject: [PATCH 1/4] usethis::use_package('cli') --- DESCRIPTION | 1 + 1 file changed, 1 insertion(+) diff --git a/DESCRIPTION b/DESCRIPTION index a5de4660..49a8d874 100644 --- a/DESCRIPTION +++ b/DESCRIPTION @@ -25,6 +25,7 @@ BugReports: https://github.com/sfirke/janitor/issues Depends: R (>= 3.1.2) Imports: + cli, dplyr (>= 1.0.0), hms, lifecycle, From 13a4ce5b2f5045b1c41db5cbba77eecadee5c741 Mon Sep 17 00:00:00 2001 From: olivroy Date: Sun, 10 Sep 2023 10:24:45 -0400 Subject: [PATCH 2/4] Improve error message of `convert_to_date()` --- R/convert_to_date.R | 37 ++++++++++----------------- tests/testthat/test-convert_to_date.R | 14 +++------- 2 files changed, 18 insertions(+), 33 deletions(-) diff --git a/R/convert_to_date.R b/R/convert_to_date.R index e124b446..cd2c7ea6 100644 --- a/R/convert_to_date.R +++ b/R/convert_to_date.R @@ -111,7 +111,7 @@ convert_to_datetime_helper.Date <- function(x, ..., tz = "UTC", out_class = c("P ret } -convert_to_datetime_helper.character <- function(x, ..., tz = "UTC", character_fun = lubridate::ymd_hms, string_conversion_failure = c("error", "warning"), out_class = c("POSIXct", "Date")) { +convert_to_datetime_helper.character <- function(x, ..., tz = "UTC", character_fun = lubridate::ymd_hms, string_conversion_failure = c("error", "warning"), out_class = c("POSIXct", "Date"), error_call = rlang::caller_env()) { string_conversion_failure <- match.arg(string_conversion_failure) out_class <- match.arg(out_class) mask_na <- is.na(x) @@ -133,35 +133,26 @@ convert_to_datetime_helper.character <- function(x, ..., tz = "UTC", character_f character_fun(x[mask_character], ...) } if (!(out_class %in% class(characters_converted))) { - stop( - "`character_fun(x)` must return class ", out_class, - "; the returned class was: ", paste(class(characters_converted), collapse = ", ") - ) + cli::cli_abort(c( + x = "`character_fun(x)` must return class {out_class}", + "The returned class was: {.obj_type_friendly {characters_converted}}" + ), + call = error_call) } ret[mask_character] <- characters_converted - if (any(is.na(ret[mask_character]))) { + if (anyNA(ret[mask_character])) { not_converted_values <- unique(x[mask_character & is.na(ret)]) - # Don't provide too many error values - if (length(not_converted_values) > 10) { - not_converted_values <- - paste( - paste0('"', not_converted_values[1:9], '"', collapse = ", "), - "... and", length(not_converted_values) - 9, "other values." - ) - } else { - not_converted_values <- - paste0('"', not_converted_values, '"', collapse = ", ") - } + n_not_converted <- length(not_converted_values) + # Let cli print which values were not converted. not_converted_message <- - paste0( - "Not all character strings converted to class ", out_class, - ". Values not converted were: ", - not_converted_values + c( + "Not all character strings converted to class {.cls {out_class}}.", + i = "{n_not_converted} value{?s} not converted: {.val {not_converted_values}}" ) if (string_conversion_failure %in% "error") { - stop(not_converted_message) + cli::cli_abort(not_converted_message, call = error_call) } else { - warning(not_converted_message) + cli::cli_warn(not_converted_message) } } } diff --git a/tests/testthat/test-convert_to_date.R b/tests/testthat/test-convert_to_date.R index fb7e8e98..a10adfe4 100644 --- a/tests/testthat/test-convert_to_date.R +++ b/tests/testthat/test-convert_to_date.R @@ -103,25 +103,19 @@ test_that("convert_date warnings and errors work", { expect_warning( expect_error( convert_to_date("A"), - regexp = "Not all character strings converted to class Date." + regexp = "Not all character strings converted to class .+Date" ), regexp = "All formats failed to parse." # lubridate warning ) expect_warning( expect_error( convert_to_date(LETTERS), - regexp = "Not all character strings converted to class Date.*17 other values", + regexp = "Not all character strings converted to class .+Date", info = "Confirm the 'other values' when there are many values not converted." ), regexp = "All formats failed to parse." # lubridate warning ) - expect_warning( - expect_error( - convert_to_date(LETTERS), - regexp = "Not all character strings converted to class Date." - ), - regexp = "All formats failed to parse." # lubridate warning - ) + expect_warning( expect_warning( expect_equal( @@ -130,7 +124,7 @@ test_that("convert_date warnings and errors work", { ), regexp = "All formats failed to parse. No formats found." ), - regexp = "Not all character strings converted to class Date." + regexp = "Not all character strings converted to class.+Date." ) expect_error( convert_to_date("A", character_fun = function(x) 1), From 3f5687f5e9d3f5097632d912dd62ccd041064bdd Mon Sep 17 00:00:00 2001 From: olivroy Date: Sun, 10 Sep 2023 10:39:09 -0400 Subject: [PATCH 3/4] Style --- R/convert_to_date.R | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/R/convert_to_date.R b/R/convert_to_date.R index cd2c7ea6..76ca4c95 100644 --- a/R/convert_to_date.R +++ b/R/convert_to_date.R @@ -133,11 +133,13 @@ convert_to_datetime_helper.character <- function(x, ..., tz = "UTC", character_f character_fun(x[mask_character], ...) } if (!(out_class %in% class(characters_converted))) { - cli::cli_abort(c( - x = "`character_fun(x)` must return class {out_class}", - "The returned class was: {.obj_type_friendly {characters_converted}}" - ), - call = error_call) + cli::cli_abort( + c( + x = "`character_fun(x)` must return class {out_class}", + "The returned class was: {.obj_type_friendly {characters_converted}}" + ), + call = error_call + ) } ret[mask_character] <- characters_converted if (anyNA(ret[mask_character])) { From 579290a5e5ab9c89c337e5e74c686e7a1f44c577 Mon Sep 17 00:00:00 2001 From: olivroy Date: Sun, 10 Sep 2023 10:45:23 -0400 Subject: [PATCH 4/4] Style --- tests/testthat/test-convert_to_date.R | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/testthat/test-convert_to_date.R b/tests/testthat/test-convert_to_date.R index a10adfe4..23a40766 100644 --- a/tests/testthat/test-convert_to_date.R +++ b/tests/testthat/test-convert_to_date.R @@ -115,7 +115,7 @@ test_that("convert_date warnings and errors work", { ), regexp = "All formats failed to parse." # lubridate warning ) - + expect_warning( expect_warning( expect_equal(