-
Notifications
You must be signed in to change notification settings - Fork 130
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
Improve error messages with cli #560
Changes from 3 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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,28 @@ 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}}" | ||
Comment on lines
+150
to
+152
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Using cli pluralization + inline markup https://cli.r-lib.org/index.html#pluralization https://cli.r-lib.org/reference/inline-markup.html. Also cli automatically makes the display of values shorter, hence the removal of the code above that prepares printing of the diagnostic. |
||
) | ||
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) | ||
} | ||
} | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 | ||
) | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. duplicated test. |
||
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), | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
to ensure that it print
Error in convert_date()
/convert_datetime()
instead of `Error in convert_to_datetime_helper