Soften rules for interpolating values (#1466)

Revert to previous escaping method (i.e. reverts #1368). See bcgov/bcdata#338 for details. If we do this again in the future, I think we have to do argument checking in individual translations. That's a lot of work but allows a whole suite of potentially useful translations for more complex R objects.

NAMESPACE

@@ -121,9 +121,11 @@ S3method(escape,Date)
 S3method(escape,POSIXt)
 S3method(escape,blob)
 S3method(escape,character)
+S3method(escape,data.frame)
 S3method(escape,dbplyr_catalog)
 S3method(escape,dbplyr_schema)
 S3method(escape,dbplyr_table_path)
+S3method(escape,default)
 S3method(escape,double)
 S3method(escape,factor)
 S3method(escape,ident)
@@ -132,6 +134,7 @@ S3method(escape,integer)
 S3method(escape,integer64)
 S3method(escape,list)
 S3method(escape,logical)
+S3method(escape,reactivevalues)
 S3method(escape,sql)
 S3method(explain,tbl_sql)
 S3method(flatten_query,base_query)

NEWS.md

@@ -37,9 +37,6 @@
 
 * The databricks backend now supports creating non-temporary tables too (#1418).
 
-* Clearer error if you attempt to embed non-atomic vectors inside of a generated 
-  query (#1368).
-
 * `x$name` never attempts to evaluate `name` (#1368).
 
 * `rows_patch(in_place = FALSE)` now works when more than one column should be

R/backend-.R

@@ -62,10 +62,11 @@ base_scalar <- sql_translator(
   },
 
   `$`   = function(x, name) {
-    if (!is.sql(x)) {
-      cli_abort("{.code $} can only subset database columns, not inlined values.")
+    if (is.sql(x)) {
+      glue_sql2(sql_current_con(), "{x}.{.col name}")
+    } else {
+      eval(bquote(`$`(x, .(substitute(name)))))
     }
-    glue_sql2(sql_current_con(), "{x}.{.col name}")
   },
 
   `[[`   = function(x, i) {

R/escape.R

@@ -137,6 +137,21 @@ escape.list <- function(x, parens = TRUE, collapse = ", ", con = NULL) {
   sql_vector(pieces, parens, collapse, con = con)
 }
 
+#' @export
+escape.data.frame <- function(x, parens = TRUE, collapse = ", ", con = NULL) {
+  error_embed("a data.frame", "df$x")
+}
+
+#' @export
+escape.reactivevalues <- function(x, parens = TRUE, collapse = ", ", con = NULL) {
+  error_embed("shiny inputs", "input$x")
+}
+
+#' @export
+escape.default <- function(x, parens = TRUE, collapse = ", ", con = NULL) {
+  error_embed(obj_type_friendly(x), "x")
+}
+
 # Also used in default_ops() for reactives
 error_embed <- function(type, expr) {
   cli_abort(c(

R/tidyeval.R

@@ -157,20 +157,13 @@ partial_eval_sym <- function(sym, data, env) {
   if (name %in% vars) {
     sym
   } else if (env_has(env, name, inherit = TRUE)) {
-    val <- eval_bare(sym, env)
-
-    # Handle common failure modes
-    if (inherits(val, "data.frame")) {
-      error_embed("a data.frame", paste0(name, "$x"))
-    } else if (inherits(val, "reactivevalues")) {
-      error_embed("shiny inputs", paste0(name, "$x"))
-    }
+    # Inline the value so that the translation function can choose what to do
 
-    if (is_sql_literal(val)) {
-      unname(val)
-    } else {
-      error_embed(obj_type_friendly(val), name)
+    val <- eval_bare(sym, env)
+    if (is_atomic(val)) {
+      val <- unname(val)
     }
+    val
   } else {
     cli::cli_abort(
       "Object {.var {name}} not found.",

revdep/README.md

@@ -16,20 +16,18 @@
 |SQLDataFrame       |?       |      |        |     |
 |synaptome.db       |?       |      |        |     |
 
-## New problems (12)
+## New problems (10)
 
-|package             |version  |error  |warning |note |
-|:-------------------|:--------|:------|:-------|:----|
-|[Andromeda](problems.md#andromeda)|0.6.5    |__+3__ |        |     |
-|[arrow](problems.md#arrow)|14.0.0.2 |__+1__ |        |2    |
-|[bcdata](problems.md#bcdata)|0.4.1    |__+1__ |        |     |
-|[CDMConnector](problems.md#cdmconnector)|1.3.0    |__+2__ |        |1    |
-|[childesr](problems.md#childesr)|0.2.3    |__+1__ |        |1    |
-|[CohortSurvival](problems.md#cohortsurvival)|0.3.0    |__+2__ |        |     |
-|[diseasystore](problems.md#diseasystore)|0.1.1    |__+1__ |        |     |
-|[DrugUtilisation](problems.md#drugutilisation)|0.5.0    |__+2__ |        |     |
-|[IncidencePrevalence](problems.md#incidenceprevalence)|0.7.0    |__+2__ |        |     |
-|[PatientProfiles](problems.md#patientprofiles)|0.6.0    |__+2__ |        |     |
-|[pool](problems.md#pool)|1.0.3    |__+2__ |        |     |
-|[SCDB](problems.md#scdb)|0.3      |__+3__ |        |     |
+|package             |version |error  |warning |note |
+|:-------------------|:-------|:------|:-------|:----|
+|[Andromeda](problems.md#andromeda)|0.6.5   |__+3__ |        |     |
+|[CDMConnector](problems.md#cdmconnector)|1.3.0   |__+2__ |        |1    |
+|[childesr](problems.md#childesr)|0.2.3   |__+1__ |        |1    |
+|[CohortSurvival](problems.md#cohortsurvival)|0.3.0   |__+2__ |        |     |
+|[diseasystore](problems.md#diseasystore)|0.1.1   |__+1__ |        |     |
+|[DrugUtilisation](problems.md#drugutilisation)|0.5.0   |__+2__ |        |     |
+|[IncidencePrevalence](problems.md#incidenceprevalence)|0.7.1   |__+2__ |        |     |
+|[PatientProfiles](problems.md#patientprofiles)|0.6.1   |__+2__ |        |     |
+|[pool](problems.md#pool)|1.0.3   |__+2__ |        |     |
+|[SCDB](problems.md#scdb)|0.3     |__+3__ |        |     |
 

revdep/cran.md

@@ -1,8 +1,8 @@
 ## revdepcheck results
 
-We checked 108 reverse dependencies (97 from CRAN + 11 from Bioconductor), comparing R CMD check results across CRAN and dev versions of this package.
+We checked 110 reverse dependencies (99 from CRAN + 11 from Bioconductor), comparing R CMD check results across CRAN and dev versions of this package.
 
- * We saw 12 new problems
+ * We saw 10 new problems
  * We failed to check 0 packages
 
 Issues with CRAN packages are summarised below.
@@ -15,12 +15,6 @@ Issues with CRAN packages are summarised below.
   checking tests ... ERROR
   checking re-building of vignette outputs ... ERROR
 
-* arrow
-  checking examples ... ERROR
-
-* bcdata
-  checking tests ... ERROR
-
 * CDMConnector
   checking tests ... ERROR
   checking re-building of vignette outputs ... ERROR