From 568bd2a188c9c73f5222a54486fa3326071d222e Mon Sep 17 00:00:00 2001 From: Hadley Wickham Date: Thu, 26 Jan 2023 15:26:48 -0600 Subject: [PATCH 1/5] Wrap glue errors, if possible Fixes #229 --- DESCRIPTION | 1 + R/sql.R | 4 ++-- R/transformer.R | 25 ++++++++++++++++++++++++- tests/testthat/_snaps/transformer.md | 17 +++++++++++++++++ tests/testthat/test-transformer.R | 6 ++++++ 5 files changed, 50 insertions(+), 3 deletions(-) create mode 100644 tests/testthat/_snaps/transformer.md create mode 100644 tests/testthat/test-transformer.R diff --git a/DESCRIPTION b/DESCRIPTION index 63515d0a..6ef0b09f 100644 --- a/DESCRIPTION +++ b/DESCRIPTION @@ -32,6 +32,7 @@ Suggests: magrittr, microbenchmark, R.utils, + rlang, rmarkdown, rprintf, RSQLite, diff --git a/R/sql.R b/R/sql.R index 8a8c2738..4dff48a8 100644 --- a/R/sql.R +++ b/R/sql.R @@ -164,7 +164,7 @@ sql_quote_transformer <- function(connection, .na) { is_quoted <- any(m[[1]] != -1) if (is_quoted) { regmatches(text, m) <- "" - res <- eval(parse(text = text, keep.source = FALSE), envir) + res <- identity_transformer(text, envir) if (length(res) == 1) { res <- DBI::dbQuoteIdentifier(conn = connection, res) @@ -174,7 +174,7 @@ sql_quote_transformer <- function(connection, .na) { res[] <- lapply(res, DBI::dbQuoteIdentifier, conn = connection) } } else { - res <- eval(parse(text = text, keep.source = FALSE), envir) + res <- identity_transformer(text, envir) if (inherits(res, "SQL")) { if (should_collapse) { res <- glue_collapse(res, ", ") diff --git a/R/transformer.R b/R/transformer.R index 0c4bade6..28848a64 100644 --- a/R/transformer.R +++ b/R/transformer.R @@ -8,5 +8,28 @@ #' custom glue transformers and some common use cases. #' @export identity_transformer <- function(text, envir) { - eval(parse(text = text, keep.source = FALSE), envir) + if (requireNamespace("rlang", quiet = TRUE)) { + tryCatch( + expr <- parse(text = text, keep.source = FALSE), + error = function(err) { + rlang::abort( + "Failed to parse glue component", + parent = err, + call = NULL + ) + } + ) + tryCatch( + eval(expr, envir), + error = function(err) { + rlang::abort( + paste0("Failed to evaluate glue component {", text, "}"), + parent = err, + call = NULL + ) + } + ) + } else { + eval(parse(text = text, keep.source = FALSE), envir) + } } diff --git a/tests/testthat/_snaps/transformer.md b/tests/testthat/_snaps/transformer.md new file mode 100644 index 00000000..13503f21 --- /dev/null +++ b/tests/testthat/_snaps/transformer.md @@ -0,0 +1,17 @@ +# get nice errors if rlang installed + + Code + identity_transformer("x + ") + Error + Failed to parse glue component + Caused by error in `parse()`: + ! :2:0: unexpected end of input + 1: x + + ^ + Code + identity_transformer("NOTFOUND") + Error + Failed to evaluate glue component {NOTFOUND} + Caused by error: + ! argument "envir" is missing, with no default + diff --git a/tests/testthat/test-transformer.R b/tests/testthat/test-transformer.R new file mode 100644 index 00000000..01ceaa41 --- /dev/null +++ b/tests/testthat/test-transformer.R @@ -0,0 +1,6 @@ +test_that("get nice errors if rlang installed", { + expect_snapshot(error = TRUE, { + identity_transformer("x + ") + identity_transformer("NOTFOUND") + }) +}) From 43963d55564eaa173745ff1c137aced0f24d2a5d Mon Sep 17 00:00:00 2001 From: Hadley Wickham Date: Mon, 27 Mar 2023 10:50:35 -0500 Subject: [PATCH 2/5] Apply suggestions from code review Co-authored-by: Davis Vaughan --- R/transformer.R | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/R/transformer.R b/R/transformer.R index 28848a64..3b19eb99 100644 --- a/R/transformer.R +++ b/R/transformer.R @@ -8,7 +8,7 @@ #' custom glue transformers and some common use cases. #' @export identity_transformer <- function(text, envir) { - if (requireNamespace("rlang", quiet = TRUE)) { + if (requireNamespace("rlang", quietly = TRUE)) { tryCatch( expr <- parse(text = text, keep.source = FALSE), error = function(err) { From 90aac8bfd19683ac525ad48862eb2e791deec60d Mon Sep 17 00:00:00 2001 From: Hadley Wickham Date: Mon, 27 Mar 2023 10:56:33 -0500 Subject: [PATCH 3/5] Feedback from code review --- NEWS.md | 3 +++ R/transformer.R | 47 ++++++++++++++++++------------------ tests/testthat/_snaps/sql.md | 17 +++++++++++++ tests/testthat/test-sql.R | 7 ++++++ 4 files changed, 51 insertions(+), 23 deletions(-) create mode 100644 tests/testthat/_snaps/sql.md diff --git a/NEWS.md b/NEWS.md index e3e829e8..9442764e 100644 --- a/NEWS.md +++ b/NEWS.md @@ -1,5 +1,8 @@ # glue (development version) +* If rlang is installed, glue will generate more informative errors if an + interpolated expression either can't be parsed or fails to evaluate (#229). + * `glue_collapse(character())` (and hence `glue_sql_collapse(character())`) now return `""`, so that they always return a single string (#88). diff --git a/R/transformer.R b/R/transformer.R index 3b19eb99..ab1b49d9 100644 --- a/R/transformer.R +++ b/R/transformer.R @@ -8,28 +8,29 @@ #' custom glue transformers and some common use cases. #' @export identity_transformer <- function(text, envir) { - if (requireNamespace("rlang", quietly = TRUE)) { - tryCatch( - expr <- parse(text = text, keep.source = FALSE), - error = function(err) { - rlang::abort( - "Failed to parse glue component", - parent = err, - call = NULL - ) - } - ) - tryCatch( - eval(expr, envir), - error = function(err) { - rlang::abort( - paste0("Failed to evaluate glue component {", text, "}"), - parent = err, - call = NULL - ) - } - ) - } else { - eval(parse(text = text, keep.source = FALSE), envir) + with_glue_error( + expr <- parse(text = text, keep.source = FALSE), + "Failed to parse glue component" + ) + with_glue_error( + eval(expr, envir), + paste0("Failed to evaluate glue component {", text, "}") + ) +} + +with_glue_error <- function(expr, message) { + if (!requireNamespace("rlang", quietly = TRUE)) { + return(expr) } + + withCallingHandlers( + expr, + error = function(cnd) { + rlang::abort( + message, + parent = cnd, + call = NULL + ) + } + ) } diff --git a/tests/testthat/_snaps/sql.md b/tests/testthat/_snaps/sql.md new file mode 100644 index 00000000..28da279e --- /dev/null +++ b/tests/testthat/_snaps/sql.md @@ -0,0 +1,17 @@ +# get nice errors if rlang installed + + Code + glue_sql("{x + }") + Error + Failed to parse glue component + Caused by error in `parse()`: + ! :2:0: unexpected end of input + 1: x + + ^ + Code + glue_sql("{NOTFOUND}") + Error + Failed to evaluate glue component {NOTFOUND} + Caused by error: + ! object 'NOTFOUND' not found + diff --git a/tests/testthat/test-sql.R b/tests/testthat/test-sql.R index 80dca538..cd1ae58c 100644 --- a/tests/testthat/test-sql.R +++ b/tests/testthat/test-sql.R @@ -130,3 +130,10 @@ describe("glue_sql_collapse", { ) }) }) + +test_that("get nice errors if rlang installed", { + expect_snapshot(error = TRUE, { + glue_sql("{x + }") + glue_sql("{NOTFOUND}") + }) +}) From a1b5a75a2a66ec5520f80784801c656b412fec9a Mon Sep 17 00:00:00 2001 From: Jenny Bryan Date: Mon, 27 Nov 2023 18:47:47 -0800 Subject: [PATCH 4/5] Have a default for `envir` --- R/transformer.R | 2 +- tests/testthat/_snaps/transformer.md | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/R/transformer.R b/R/transformer.R index ab1b49d9..2ac34b7a 100644 --- a/R/transformer.R +++ b/R/transformer.R @@ -7,7 +7,7 @@ #' @seealso `vignette("transformers", "glue")` for documentation on creating #' custom glue transformers and some common use cases. #' @export -identity_transformer <- function(text, envir) { +identity_transformer <- function(text, envir = parent.frame()) { with_glue_error( expr <- parse(text = text, keep.source = FALSE), "Failed to parse glue component" diff --git a/tests/testthat/_snaps/transformer.md b/tests/testthat/_snaps/transformer.md index 62fc32fd..ffbb48ab 100644 --- a/tests/testthat/_snaps/transformer.md +++ b/tests/testthat/_snaps/transformer.md @@ -14,6 +14,6 @@ Condition Error: ! Failed to evaluate glue component {NOTFOUND} - Caused by error in `identity_transformer()`: - ! argument "envir" is missing, with no default + Caused by error: + ! object 'NOTFOUND' not found From 4eefffb42f5eabe33f5fd209aaadb1ce0c4e9b4e Mon Sep 17 00:00:00 2001 From: Hadley Wickham Date: Tue, 28 Nov 2023 07:08:44 -0600 Subject: [PATCH 5/5] Fix merge (?) mistake --- DESCRIPTION | 2 -- 1 file changed, 2 deletions(-) diff --git a/DESCRIPTION b/DESCRIPTION index 1a21fd2a..fb0d38c2 100644 --- a/DESCRIPTION +++ b/DESCRIPTION @@ -27,8 +27,6 @@ Suggests: dplyr, knitr, magrittr, - microbenchmark, - R.utils, rlang, rmarkdown, RSQLite,