From a352cdc8a21de6f2954f12889be50fe8ee709470 Mon Sep 17 00:00:00 2001 From: Hadley Wickham Date: Fri, 23 Aug 2024 08:48:28 -0500 Subject: [PATCH 1/2] Use rlang for error wrapping, if available This includes some general improvements to the erorr handling, e.g. using `conditionMessage()` instead of `$message` and `withCallingHandlers()` instead of `tryCatch()`. I also made the hint conditional, since I've seen it a couple of times when it wasn't appropriate, and it's better not to send the user off chasing a red herring. --- DESCRIPTION | 5 +- NEWS.md | 1 + R/formatters.R | 82 ++++++++++++++++++++--------- tests/testthat/_snaps/formatters.md | 19 +++++-- tests/testthat/test-formatters.R | 3 ++ 5 files changed, 79 insertions(+), 31 deletions(-) diff --git a/DESCRIPTION b/DESCRIPTION index 02ae9f35..2576d2d5 100644 --- a/DESCRIPTION +++ b/DESCRIPTION @@ -14,7 +14,7 @@ Description: Inspired by the the 'futile.logger' R package and 'logging' License: AGPL-3 URL: https://daroczig.github.io/logger/ BugReports: https://github.com/daroczig/logger/issues -Depends: +Depends: R (>= 4.0.0) Imports: utils @@ -30,6 +30,7 @@ Suggests: pander, parallel, R.utils, + rlang, rmarkdown, roxygen2, RPushbullet, @@ -45,7 +46,7 @@ Enhances: futile.logger, log4r, logging -VignetteBuilder: +VignetteBuilder: knitr Config/testthat/edition: 3 Config/testthat/parallel: TRUE diff --git a/NEWS.md b/NEWS.md index 67e79efe..5dad5e2f 100644 --- a/NEWS.md +++ b/NEWS.md @@ -1,5 +1,6 @@ # logger (development version) +* If rlang is installed, errors in `formatter_glue()` and `formatter_glue_safe()` get improved appearance. * `log_appender()`, `log_layout()` and `log_formatter()` now check that you are calling them with a function, and return the previously set value. # logger 0.3.0 (2024-03-03) diff --git a/R/formatters.R b/R/formatters.R index 92f7ff6a..32c04b83 100644 --- a/R/formatters.R +++ b/R/formatters.R @@ -44,21 +44,12 @@ formatter_glue <- function(..., .topenv = parent.frame()) { fail_on_missing_package("glue") - withCallingHandlers( + out <- wrap_glue( glue::glue(..., .envir = .topenv), - error = function(e) { - args <- paste0(capture.output(str(...)), collapse = "\n") - - stop(paste0( - "`glue` failed in `formatter_glue` on:\n\n", - args, - "\n\nRaw error message:\n\n", - conditionMessage(e), - "\n\nPlease consider using another `log_formatter` or ", - "`skip_formatter` on strings with curly braces." - )) - } + glue_fun = "glue", + logger_fun = "formatter_glue" ) + as.character(out) } attr(formatter_glue, "generator") <- quote(formatter_glue()) @@ -75,24 +66,65 @@ formatter_glue_safe <- function(..., .topcall = sys.call(-1), .topenv = parent.frame()) { fail_on_missing_package("glue") - as.character( - tryCatch( - glue::glue_safe(..., .envir = .topenv), - error = function(e) { - stop(paste( - "`glue_safe` failed in `formatter_glue_safe` on:\n\n", - capture.output(str(...)), + + out <- wrap_glue( + glue::glue_safe(..., .envir = .topenv), + glue_fun = "glue_safe", + logger_fun = "formatter_glue_safe" + ) + as.character(out) +} +attr(formatter_glue_safe, "generator") <- quote(formatter_glue_safe()) + +wrap_glue <- function(code, glue_fun, logger_fun) { + if (has_rlang()) { + withCallingHandlers( + code, + error = function(cnd) { + rlang::abort( + c( + glue::glue("`{glue_fun}()` failed."), + i = glue_hint(conditionMessage(cnd)) + ), + parent = cnd, + call = rlang::call2(logger_fun) + ) + } + ) + } else { + code_ <- substitute(code) + + withCallingHandlers( + code, + error = function(cnd) { + hint <- glue_hint(conditionMessage(cnd)) + if (!is.null(hint)) hint <- paste0("\n\n", hint) + + stop(paste0( + "`", glue_fun, "()` failed in `", logger_fun, "()` on:\n\n", + deparse1(code_), "\n\nRaw error message:\n\n", - e$message, - "\n\nPlease consider using another `log_formatter` or", - "`skip_formatter` on strings with curly braces." - )) + conditionMessage(cnd), + hint + ), call. = FALSE) } ) + } +} + +glue_hint <- function(message) { + if (!grepl("}", message, fixed = TRUE)) { + return() + } + paste0( + "For strings containing `{`/`}` consider using", + "`skip_formatter()` or another `log_formatter`" ) } -attr(formatter_glue_safe, "generator") <- quote(formatter_glue_safe()) +has_rlang <- function() { + requireNamespace("rlang", quietly = TRUE) +} #' Apply `glue` and `sprintf` #' diff --git a/tests/testthat/_snaps/formatters.md b/tests/testthat/_snaps/formatters.md index 4f8811af..fd0767ac 100644 --- a/tests/testthat/_snaps/formatters.md +++ b/tests/testthat/_snaps/formatters.md @@ -3,14 +3,25 @@ Code log_info("hi{") Condition - Error in `h()`: - ! `glue` failed in `formatter_glue` on: + Error in `formatter_glue()`: + ! `glue()` failed. + i For strings containing `{`/`}` consider using`skip_formatter()` or another `log_formatter` + Caused by error in `glue_data()`: + ! Expecting '}' + +--- + + Code + log_info("hi{") + Condition + Error: + ! `glue()` failed in `formatter_glue()` on: - chr "hi{" + glue::glue(..., .envir = .topenv) Raw error message: Expecting '}' - Please consider using another `log_formatter` or `skip_formatter` on strings with curly braces. + For strings containing `{`/`}` consider using`skip_formatter()` or another `log_formatter` diff --git a/tests/testthat/test-formatters.R b/tests/testthat/test-formatters.R index 5b5a7893..d6a2485d 100644 --- a/tests/testthat/test-formatters.R +++ b/tests/testthat/test-formatters.R @@ -38,6 +38,9 @@ test_that("glue works", { test_that("glue gives informative error if message contains curlies", { local_test_logger(formatter = formatter_glue) expect_snapshot(log_info("hi{"), error = TRUE) + + local_mocked_bindings(has_rlang = function() FALSE) + expect_snapshot(log_info("hi{"), error = TRUE) }) test_that("glue_safe works", { From 1021368edbb2d0157e105d78ef9f3bb89f78a009 Mon Sep 17 00:00:00 2001 From: Hadley Wickham Date: Fri, 23 Aug 2024 08:52:14 -0500 Subject: [PATCH 2/2] Polishing --- R/formatters.R | 12 ++++++------ tests/testthat/_snaps/formatters.md | 6 +++--- 2 files changed, 9 insertions(+), 9 deletions(-) diff --git a/R/formatters.R b/R/formatters.R index 32c04b83..7526dd8e 100644 --- a/R/formatters.R +++ b/R/formatters.R @@ -46,6 +46,7 @@ formatter_glue <- function(..., out <- wrap_glue( glue::glue(..., .envir = .topenv), + inputs = list(...), glue_fun = "glue", logger_fun = "formatter_glue" ) @@ -69,6 +70,7 @@ formatter_glue_safe <- function(..., out <- wrap_glue( glue::glue_safe(..., .envir = .topenv), + inputs = list(...), glue_fun = "glue_safe", logger_fun = "formatter_glue_safe" ) @@ -76,7 +78,7 @@ formatter_glue_safe <- function(..., } attr(formatter_glue_safe, "generator") <- quote(formatter_glue_safe()) -wrap_glue <- function(code, glue_fun, logger_fun) { +wrap_glue <- function(code, inputs, glue_fun, logger_fun) { if (has_rlang()) { withCallingHandlers( code, @@ -92,8 +94,6 @@ wrap_glue <- function(code, glue_fun, logger_fun) { } ) } else { - code_ <- substitute(code) - withCallingHandlers( code, error = function(cnd) { @@ -102,7 +102,7 @@ wrap_glue <- function(code, glue_fun, logger_fun) { stop(paste0( "`", glue_fun, "()` failed in `", logger_fun, "()` on:\n\n", - deparse1(code_), + paste0(capture.output(str(inputs)), collapse = ""), "\n\nRaw error message:\n\n", conditionMessage(cnd), hint @@ -117,8 +117,8 @@ glue_hint <- function(message) { return() } paste0( - "For strings containing `{`/`}` consider using", - "`skip_formatter()` or another `log_formatter`" + "For strings containing `{` or `}` consider using ", + "`skip_formatter()` or another `log_formatter`." ) } diff --git a/tests/testthat/_snaps/formatters.md b/tests/testthat/_snaps/formatters.md index fd0767ac..43f4f63b 100644 --- a/tests/testthat/_snaps/formatters.md +++ b/tests/testthat/_snaps/formatters.md @@ -5,7 +5,7 @@ Condition Error in `formatter_glue()`: ! `glue()` failed. - i For strings containing `{`/`}` consider using`skip_formatter()` or another `log_formatter` + i For strings containing `{` or `}` consider using `skip_formatter()` or another `log_formatter`. Caused by error in `glue_data()`: ! Expecting '}' @@ -17,11 +17,11 @@ Error: ! `glue()` failed in `formatter_glue()` on: - glue::glue(..., .envir = .topenv) + List of 1 $ : chr "hi{" Raw error message: Expecting '}' - For strings containing `{`/`}` consider using`skip_formatter()` or another `log_formatter` + For strings containing `{` or `}` consider using `skip_formatter()` or another `log_formatter`.