From b09ded903536ab814237425f8dec298c7860707b Mon Sep 17 00:00:00 2001 From: June Choe Date: Tue, 17 Oct 2023 09:13:32 -0400 Subject: [PATCH 01/42] streamlining internal tidyselect first draft --- R/utils.R | 76 +++++++++++++++---------------------------------------- 1 file changed, 21 insertions(+), 55 deletions(-) diff --git a/R/utils.R b/R/utils.R index 68fa851ee..6cd6c6ba1 100644 --- a/R/utils.R +++ b/R/utils.R @@ -218,68 +218,34 @@ materialize_table <- function(tbl, check = TRUE) { tbl } -resolve_expr_to_cols <- function(tbl, var_expr) { - - var_expr <- enquo(var_expr) - - if ((var_expr %>% rlang::get_expr() %>% as.character())[1] == "vars") { - - cols <- (var_expr %>% rlang::get_expr() %>% as.character())[-1] - return(cols) - } - - tidyselect::vars_select(.vars = colnames(tbl), {{ var_expr }}) %>% unname() -} - -resolve_columns <- function(x, var_expr, preconditions) { - - # If getting a character vector as `var_expr`, simply return the vector - # since this should already be a vector of column names and it's not necessary - # to resolve this against the target table - if (is.character(var_expr)) { - return(var_expr) - } - - # nocov start +resolve_columns <- function(x, var_expr, preconditions, ..., call = rlang::caller_env()) { # Return an empty character vector if the expr is NULL - if (inherits(var_expr, "quosure") && - var_expr %>% rlang::as_label() == "NULL") { - + if (is.null(var_expr)) { return(character(NA_character_)) } - # nocov end + # Extract tbl and apply preconditions + tbl <- if (is_ptblank_agent(x)) get_tbl_object(x) else x + if (!is.null(preconditions)) { + tbl <- apply_preconditions(tbl = x, preconditions = preconditions) + } - # Get the column names from a non-NULL, non-character expression - if (is.null(preconditions)) { - - if (inherits(x, c("data.frame", "tbl_df", "tbl_dbi"))) { - - column <- resolve_expr_to_cols(tbl = x, var_expr = !!var_expr) - - } else if (inherits(x, ("ptblank_agent"))) { - - tbl <- get_tbl_object(agent = x) - column <- resolve_expr_to_cols(tbl = tbl, var_expr = !!var_expr) - } - + # For simplicity, get the expression out of the quosure + col_expr <- rlang::quo_get_expr(var_expr) + + # Revised column selection logic + ## Special case `vars()`-style enquo-ing and implement backwards compatibility + if (rlang::is_call(col_expr, "vars")) { + col_syms <- rlang::call_args(col_expr) + # Turn it into `c(...)` expression and forward to tidyselect + col_c_expr <- rlang::call2("c", !!!col_syms) + column <- tidyselect::eval_select(col_c_expr, tbl, error_call = call) + column <- names(column) } else { - - if (inherits(x, c("data.frame", "tbl_df", "tbl_dbi"))) { - - tbl <- apply_preconditions(tbl = x, preconditions = preconditions) - - column <- resolve_expr_to_cols(tbl = tbl, var_expr = !!var_expr) - - } else if (inherits(x, ("ptblank_agent"))) { - - tbl <- get_tbl_object(agent = x) - - tbl <- apply_preconditions(tbl = tbl, preconditions = preconditions) - - column <- resolve_expr_to_cols(tbl = tbl, var_expr = !!var_expr) - } + ## Else, proceed with the assumption that user supplied a {tidyselect} expression + column <- tidyselect::eval_select(col_expr, tbl, error_call = call) + column <- names(column) } if (length(column) < 1) { From 5cedd2008242bb023fa29046ad6f85e9d28c0ccf Mon Sep 17 00:00:00 2001 From: June Choe Date: Tue, 17 Oct 2023 09:14:06 -0400 Subject: [PATCH 02/42] [temp] make error msg regex test generally for column-related error --- tests/testthat/test-expectation_fns.R | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/tests/testthat/test-expectation_fns.R b/tests/testthat/test-expectation_fns.R index 0b0fa6017..c0a340d9f 100644 --- a/tests/testthat/test-expectation_fns.R +++ b/tests/testthat/test-expectation_fns.R @@ -1624,7 +1624,8 @@ test_that("pointblank expectation function produce the correct results", { test_that("expect errors to be expressed by pointblank under some conditions", { - no_col_msg <- "The value for `column` doesn't correspond to a column name." + # no_col_msg <- "The value for `column` doesn't correspond to a column name." + no_col_msg <- "column" # Errors caught and expressed when a column doesn't exist expect_error(expect_col_vals_lt(tbl, columns = vars(z), value = 0), regexp = no_col_msg) From 2d939aab846cd31175c63839f1712b42c17d69c7 Mon Sep 17 00:00:00 2001 From: June Choe Date: Tue, 17 Oct 2023 09:35:43 -0400 Subject: [PATCH 03/42] keep original deparse handling of vars --- R/utils.R | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/R/utils.R b/R/utils.R index 6cd6c6ba1..59fc3a0c3 100644 --- a/R/utils.R +++ b/R/utils.R @@ -237,11 +237,9 @@ resolve_columns <- function(x, var_expr, preconditions, ..., call = rlang::calle # Revised column selection logic ## Special case `vars()`-style enquo-ing and implement backwards compatibility if (rlang::is_call(col_expr, "vars")) { - col_syms <- rlang::call_args(col_expr) - # Turn it into `c(...)` expression and forward to tidyselect - col_c_expr <- rlang::call2("c", !!!col_syms) - column <- tidyselect::eval_select(col_c_expr, tbl, error_call = call) - column <- names(column) + cols <- rlang::call_args(col_expr) + # Deparse into character vector + column <- vapply(cols, rlang::as_name, character(1), USE.NAMES = FALSE) } else { ## Else, proceed with the assumption that user supplied a {tidyselect} expression column <- tidyselect::eval_select(col_expr, tbl, error_call = call) From 3f7cce44b5e38d60b149f738703d9c4909e41823 Mon Sep 17 00:00:00 2001 From: June Choe Date: Tue, 17 Oct 2023 09:54:52 -0400 Subject: [PATCH 04/42] be intentional about passing character vector of columns --- R/col_exists.R | 4 ++-- R/col_is_character.R | 4 ++-- R/col_is_date.R | 4 ++-- R/col_is_factor.R | 4 ++-- R/col_is_integer.R | 4 ++-- R/col_is_logical.R | 4 ++-- R/col_is_numeric.R | 4 ++-- R/col_is_posix.R | 4 ++-- R/col_vals_between.R | 4 ++-- R/col_vals_decreasing.R | 4 ++-- R/col_vals_equal.R | 4 ++-- R/col_vals_gt.R | 4 ++-- R/col_vals_gte.R | 4 ++-- R/col_vals_in_set.R | 4 ++-- R/col_vals_increasing.R | 4 ++-- R/col_vals_lt.R | 4 ++-- R/col_vals_lte.R | 4 ++-- R/col_vals_make_set.R | 4 ++-- R/col_vals_make_subset.R | 4 ++-- R/col_vals_not_between.R | 4 ++-- R/col_vals_not_equal.R | 4 ++-- R/col_vals_not_in_set.R | 4 ++-- R/col_vals_not_null.R | 4 ++-- R/col_vals_null.R | 4 ++-- R/col_vals_regex.R | 4 ++-- R/col_vals_within_spec.R | 4 ++-- 26 files changed, 52 insertions(+), 52 deletions(-) diff --git a/R/col_exists.R b/R/col_exists.R index c6d752841..32e989ba7 100644 --- a/R/col_exists.R +++ b/R/col_exists.R @@ -251,7 +251,7 @@ col_exists <- function( secret_agent <- create_agent(x, label = "::QUIET::") %>% col_exists( - columns = columns, + columns = tidyselect::all_of(columns), actions = prime_actions(actions), label = label, brief = brief, @@ -289,7 +289,7 @@ col_exists <- function( assertion_type = "col_exists", i_o = i_o, columns_expr = columns_expr, - column = columns[i], + column = tidyselect::all_of(columns[i]), preconditions = NULL, actions = covert_actions(actions, agent), step_id = step_id[i], diff --git a/R/col_is_character.R b/R/col_is_character.R index 97afe6d62..685926192 100644 --- a/R/col_is_character.R +++ b/R/col_is_character.R @@ -242,7 +242,7 @@ col_is_character <- function( secret_agent <- create_agent(x, label = "::QUIET::") %>% col_is_character( - columns = columns, + columns = tidyselect::all_of(columns), label = label, brief = brief, actions = prime_actions(actions), @@ -282,7 +282,7 @@ col_is_character <- function( assertion_type = "col_is_character", i_o = i_o, columns_expr = columns_expr, - column = columns[i], + column = tidyselect::all_of(columns[i]), preconditions = NULL, actions = covert_actions(actions, agent), step_id = step_id[i], diff --git a/R/col_is_date.R b/R/col_is_date.R index 3181f4e3c..ee3bed91a 100644 --- a/R/col_is_date.R +++ b/R/col_is_date.R @@ -234,7 +234,7 @@ col_is_date <- function( secret_agent <- create_agent(x, label = "::QUIET::") %>% col_is_date( - columns = columns, + columns = tidyselect::all_of(columns), label = label, brief = brief, actions = prime_actions(actions), @@ -274,7 +274,7 @@ col_is_date <- function( assertion_type = "col_is_date", i_o = i_o, columns_expr = columns_expr, - column = columns[i], + column = tidyselect::all_of(columns[i]), preconditions = NULL, actions = covert_actions(actions, agent), step_id = step_id[i], diff --git a/R/col_is_factor.R b/R/col_is_factor.R index 7283ca6c6..7d8d2e8f2 100644 --- a/R/col_is_factor.R +++ b/R/col_is_factor.R @@ -240,7 +240,7 @@ col_is_factor <- function( secret_agent <- create_agent(x, label = "::QUIET::") %>% col_is_factor( - columns = columns, + columns = tidyselect::all_of(columns), label = label, brief = brief, actions = prime_actions(actions), @@ -280,7 +280,7 @@ col_is_factor <- function( assertion_type = "col_is_factor", i_o = i_o, columns_expr = columns_expr, - column = columns[i], + column = tidyselect::all_of(columns[i]), preconditions = NULL, actions = covert_actions(actions, agent), step_id = step_id[i], diff --git a/R/col_is_integer.R b/R/col_is_integer.R index 4247b8cfc..f97500d1a 100644 --- a/R/col_is_integer.R +++ b/R/col_is_integer.R @@ -238,7 +238,7 @@ col_is_integer <- function( secret_agent <- create_agent(x, label = "::QUIET::") %>% col_is_integer( - columns = columns, + columns = tidyselect::all_of(columns), label = label, brief = brief, actions = prime_actions(actions), @@ -278,7 +278,7 @@ col_is_integer <- function( assertion_type = "col_is_integer", i_o = i_o, columns_expr = columns_expr, - column = columns[i], + column = tidyselect::all_of(columns[i]), preconditions = NULL, actions = covert_actions(actions, agent), step_id = step_id[i], diff --git a/R/col_is_logical.R b/R/col_is_logical.R index a3a4c3195..7ff74dfa0 100644 --- a/R/col_is_logical.R +++ b/R/col_is_logical.R @@ -235,7 +235,7 @@ col_is_logical <- function( secret_agent <- create_agent(x, label = "::QUIET::") %>% col_is_logical( - columns = columns, + columns = tidyselect::all_of(columns), label = label, brief = brief, actions = prime_actions(actions), @@ -275,7 +275,7 @@ col_is_logical <- function( assertion_type = "col_is_logical", i_o = i_o, columns_expr = columns_expr, - column = columns[i], + column = tidyselect::all_of(columns[i]), preconditions = NULL, actions = covert_actions(actions, agent), step_id = step_id[i], diff --git a/R/col_is_numeric.R b/R/col_is_numeric.R index a92ec6675..72f05928b 100644 --- a/R/col_is_numeric.R +++ b/R/col_is_numeric.R @@ -235,7 +235,7 @@ col_is_numeric <- function( secret_agent <- create_agent(x, label = "::QUIET::") %>% col_is_numeric( - columns = columns, + columns = tidyselect::all_of(columns), label = label, brief = brief, actions = prime_actions(actions), @@ -275,7 +275,7 @@ col_is_numeric <- function( assertion_type = "col_is_numeric", i_o = i_o, columns_expr = columns_expr, - column = columns[i], + column = tidyselect::all_of(columns[i]), preconditions = NULL, actions = covert_actions(actions, agent), step_id = step_id[i], diff --git a/R/col_is_posix.R b/R/col_is_posix.R index 49a1b8926..dc442249c 100644 --- a/R/col_is_posix.R +++ b/R/col_is_posix.R @@ -235,7 +235,7 @@ col_is_posix <- function( secret_agent <- create_agent(x, label = "::QUIET::") %>% col_is_posix( - columns = columns, + columns = tidyselect::all_of(columns), label = label, brief = brief, actions = prime_actions(actions), @@ -275,7 +275,7 @@ col_is_posix <- function( assertion_type = "col_is_posix", i_o = i_o, columns_expr = columns_expr, - column = columns[i], + column = tidyselect::all_of(columns[i]), preconditions = NULL, actions = covert_actions(actions, agent), step_id = step_id[i], diff --git a/R/col_vals_between.R b/R/col_vals_between.R index e4d21464f..f51bdc705 100644 --- a/R/col_vals_between.R +++ b/R/col_vals_between.R @@ -393,7 +393,7 @@ col_vals_between <- function( secret_agent <- create_agent(x, label = "::QUIET::") %>% col_vals_between( - columns = columns, + columns = tidyselect::all_of(columns), left = left, right = right, inclusive = inclusive, @@ -448,7 +448,7 @@ col_vals_between <- function( assertion_type = "col_vals_between", i_o = i_o, columns_expr = columns_expr, - column = columns[i], + column = tidyselect::all_of(columns[i]), values = c(left, right), na_pass = na_pass, preconditions = preconditions, diff --git a/R/col_vals_decreasing.R b/R/col_vals_decreasing.R index 21efe8729..5097a5de9 100644 --- a/R/col_vals_decreasing.R +++ b/R/col_vals_decreasing.R @@ -379,7 +379,7 @@ col_vals_decreasing <- function( secret_agent <- create_agent(x, label = "::QUIET::") %>% col_vals_decreasing( - columns = columns, + columns = tidyselect::all_of(columns), allow_stationary = allow_stationary, increasing_tol = increasing_tol, na_pass = na_pass, @@ -433,7 +433,7 @@ col_vals_decreasing <- function( assertion_type = "col_vals_decreasing", i_o = i_o, columns_expr = columns_expr, - column = columns[i], + column = tidyselect::all_of(columns[i]), values = stat_tol, na_pass = na_pass, preconditions = preconditions, diff --git a/R/col_vals_equal.R b/R/col_vals_equal.R index f8ffd8804..04abb7166 100644 --- a/R/col_vals_equal.R +++ b/R/col_vals_equal.R @@ -332,7 +332,7 @@ col_vals_equal <- function( secret_agent <- create_agent(x, label = "::QUIET::") %>% col_vals_equal( - columns = columns, + columns = tidyselect::all_of(columns), value = value, na_pass = na_pass, preconditions = preconditions, @@ -385,7 +385,7 @@ col_vals_equal <- function( assertion_type = "col_vals_equal", i_o = i_o, columns_expr = columns_expr, - column = columns[i], + column = tidyselect::all_of(columns[i]), values = value, na_pass = na_pass, preconditions = preconditions, diff --git a/R/col_vals_gt.R b/R/col_vals_gt.R index 1ebbd99e8..09c128e82 100644 --- a/R/col_vals_gt.R +++ b/R/col_vals_gt.R @@ -452,7 +452,7 @@ col_vals_gt <- function( secret_agent <- create_agent(x, label = "::QUIET::") %>% col_vals_gt( - columns = columns, + columns = tidyselect::all_of(columns), value = value, na_pass = na_pass, preconditions = preconditions, @@ -505,7 +505,7 @@ col_vals_gt <- function( assertion_type = "col_vals_gt", i_o = i_o, columns_expr = columns_expr, - column = columns[i], + column = tidyselect::all_of(columns[i]), values = value, na_pass = na_pass, preconditions = preconditions, diff --git a/R/col_vals_gte.R b/R/col_vals_gte.R index 5b6e14109..3f2691694 100644 --- a/R/col_vals_gte.R +++ b/R/col_vals_gte.R @@ -331,7 +331,7 @@ col_vals_gte <- function( secret_agent <- create_agent(x, label = "::QUIET::") %>% col_vals_gte( - columns = columns, + columns = tidyselect::all_of(columns), value = value, na_pass = na_pass, preconditions = preconditions, @@ -384,7 +384,7 @@ col_vals_gte <- function( assertion_type = "col_vals_gte", i_o = i_o, columns_expr = columns_expr, - column = columns[i], + column = tidyselect::all_of(columns[i]), values = value, na_pass = na_pass, preconditions = preconditions, diff --git a/R/col_vals_in_set.R b/R/col_vals_in_set.R index e4b7557cc..564837179 100644 --- a/R/col_vals_in_set.R +++ b/R/col_vals_in_set.R @@ -325,7 +325,7 @@ col_vals_in_set <- function( secret_agent <- create_agent(x, label = "::QUIET::") %>% col_vals_in_set( - columns = columns, + columns = tidyselect::all_of(columns), set = set, preconditions = preconditions, segments = segments, @@ -377,7 +377,7 @@ col_vals_in_set <- function( assertion_type = "col_vals_in_set", i_o = i_o, columns_expr = columns_expr, - column = columns[i], + column = tidyselect::all_of(columns[i]), values = set, preconditions = preconditions, seg_expr = segments, diff --git a/R/col_vals_increasing.R b/R/col_vals_increasing.R index 7d77bf6d9..521cb0db3 100644 --- a/R/col_vals_increasing.R +++ b/R/col_vals_increasing.R @@ -367,7 +367,7 @@ col_vals_increasing <- function( secret_agent <- create_agent(x, label = "::QUIET::") %>% col_vals_increasing( - columns = columns, + columns = tidyselect::all_of(columns), allow_stationary = allow_stationary, decreasing_tol = decreasing_tol, na_pass = na_pass, @@ -421,7 +421,7 @@ col_vals_increasing <- function( assertion_type = "col_vals_increasing", i_o = i_o, columns_expr = columns_expr, - column = columns[i], + column = tidyselect::all_of(columns[i]), values = stat_tol, na_pass = na_pass, preconditions = preconditions, diff --git a/R/col_vals_lt.R b/R/col_vals_lt.R index d8553a2b0..a94ab9910 100644 --- a/R/col_vals_lt.R +++ b/R/col_vals_lt.R @@ -333,7 +333,7 @@ col_vals_lt <- function( secret_agent <- create_agent(x, label = "::QUIET::") %>% col_vals_lt( - columns = columns, + columns = tidyselect::all_of(columns), value = value, na_pass = na_pass, preconditions = preconditions, @@ -386,7 +386,7 @@ col_vals_lt <- function( assertion_type = "col_vals_lt", i_o = i_o, columns_expr = columns_expr, - column = columns[i], + column = tidyselect::all_of(columns[i]), values = value, na_pass = na_pass, preconditions = preconditions, diff --git a/R/col_vals_lte.R b/R/col_vals_lte.R index 56f14b4c2..79f0d800c 100644 --- a/R/col_vals_lte.R +++ b/R/col_vals_lte.R @@ -334,7 +334,7 @@ col_vals_lte <- function( secret_agent <- create_agent(x, label = "::QUIET::") %>% col_vals_lte( - columns = columns, + columns = tidyselect::all_of(columns), value = value, na_pass = na_pass, preconditions = preconditions, @@ -387,7 +387,7 @@ col_vals_lte <- function( assertion_type = "col_vals_lte", i_o = i_o, columns_expr = columns_expr, - column = columns[i], + column = tidyselect::all_of(columns[i]), values = value, na_pass = na_pass, preconditions = preconditions, diff --git a/R/col_vals_make_set.R b/R/col_vals_make_set.R index f6323ab52..81bde081b 100644 --- a/R/col_vals_make_set.R +++ b/R/col_vals_make_set.R @@ -327,7 +327,7 @@ col_vals_make_set <- function( secret_agent <- create_agent(x, label = "::QUIET::") %>% col_vals_make_set( - columns = columns, + columns = tidyselect::all_of(columns), set = set, preconditions = preconditions, segments = segments, @@ -379,7 +379,7 @@ col_vals_make_set <- function( assertion_type = "col_vals_make_set", i_o = i_o, columns_expr = columns_expr, - column = columns[i], + column = tidyselect::all_of(columns[i]), values = set, preconditions = preconditions, seg_expr = segments, diff --git a/R/col_vals_make_subset.R b/R/col_vals_make_subset.R index e88fbe6d3..49ecfc234 100644 --- a/R/col_vals_make_subset.R +++ b/R/col_vals_make_subset.R @@ -324,7 +324,7 @@ col_vals_make_subset <- function( secret_agent <- create_agent(x, label = "::QUIET::") %>% col_vals_make_subset( - columns = columns, + columns = tidyselect::all_of(columns), set = set, preconditions = preconditions, segments = segments, @@ -376,7 +376,7 @@ col_vals_make_subset <- function( assertion_type = "col_vals_make_subset", i_o = i_o, columns_expr = columns_expr, - column = columns[i], + column = tidyselect::all_of(columns[i]), values = set, preconditions = preconditions, seg_expr = segments, diff --git a/R/col_vals_not_between.R b/R/col_vals_not_between.R index ccc12c229..96ea45941 100644 --- a/R/col_vals_not_between.R +++ b/R/col_vals_not_between.R @@ -396,7 +396,7 @@ col_vals_not_between <- function( secret_agent <- create_agent(x, label = "::QUIET::") %>% col_vals_not_between( - columns = columns, + columns = tidyselect::all_of(columns), left = left, right = right, inclusive = inclusive, @@ -451,7 +451,7 @@ col_vals_not_between <- function( assertion_type = "col_vals_not_between", i_o = i_o, columns_expr = columns_expr, - column = columns[i], + column = tidyselect::all_of(columns[i]), values = c(left, right), na_pass = na_pass, preconditions = preconditions, diff --git a/R/col_vals_not_equal.R b/R/col_vals_not_equal.R index 43ecef4c4..5436dc1cd 100644 --- a/R/col_vals_not_equal.R +++ b/R/col_vals_not_equal.R @@ -331,7 +331,7 @@ col_vals_not_equal <- function( secret_agent <- create_agent(x, label = "::QUIET::") %>% col_vals_not_equal( - columns = columns, + columns = tidyselect::all_of(columns), value = value, na_pass = na_pass, preconditions = preconditions, @@ -384,7 +384,7 @@ col_vals_not_equal <- function( assertion_type = "col_vals_not_equal", i_o = i_o, columns_expr = columns_expr, - column = columns[i], + column = tidyselect::all_of(columns[i]), values = value, na_pass = na_pass, preconditions = preconditions, diff --git a/R/col_vals_not_in_set.R b/R/col_vals_not_in_set.R index 6595770bf..1f4ea2e23 100644 --- a/R/col_vals_not_in_set.R +++ b/R/col_vals_not_in_set.R @@ -321,7 +321,7 @@ col_vals_not_in_set <- function( secret_agent <- create_agent(x, label = "::QUIET::") %>% col_vals_not_in_set( - columns = columns, + columns = tidyselect::all_of(columns), set = set, preconditions = preconditions, segments = segments, @@ -373,7 +373,7 @@ col_vals_not_in_set <- function( assertion_type = "col_vals_not_in_set", i_o = i_o, columns_expr = columns_expr, - column = columns[i], + column = tidyselect::all_of(columns[i]), values = set, preconditions = preconditions, seg_expr = segments, diff --git a/R/col_vals_not_null.R b/R/col_vals_not_null.R index 4065aad83..90e7c7d59 100644 --- a/R/col_vals_not_null.R +++ b/R/col_vals_not_null.R @@ -312,7 +312,7 @@ col_vals_not_null <- function( secret_agent <- create_agent(x, label = "::QUIET::") %>% col_vals_not_null( - columns = columns, + columns = tidyselect::all_of(columns), preconditions = preconditions, segments = segments, label = label, @@ -363,7 +363,7 @@ col_vals_not_null <- function( assertion_type = "col_vals_not_null", i_o = i_o, columns_expr = columns_expr, - column = columns[i], + column = tidyselect::all_of(columns[i]), preconditions = preconditions, seg_expr = segments, seg_col = seg_col, diff --git a/R/col_vals_null.R b/R/col_vals_null.R index 258a436c7..d18b93d57 100644 --- a/R/col_vals_null.R +++ b/R/col_vals_null.R @@ -311,7 +311,7 @@ col_vals_null <- function( secret_agent <- create_agent(x, label = "::QUIET::") %>% col_vals_null( - columns = columns, + columns = tidyselect::all_of(columns), preconditions = preconditions, segments = segments, label = label, @@ -362,7 +362,7 @@ col_vals_null <- function( assertion_type = "col_vals_null", i_o = i_o, columns_expr = columns_expr, - column = columns[i], + column = tidyselect::all_of(columns[i]), preconditions = preconditions, seg_expr = segments, seg_col = seg_col, diff --git a/R/col_vals_regex.R b/R/col_vals_regex.R index 80a567388..9f49384e8 100644 --- a/R/col_vals_regex.R +++ b/R/col_vals_regex.R @@ -325,7 +325,7 @@ col_vals_regex <- function( secret_agent <- create_agent(x, label = "::QUIET::") %>% col_vals_regex( - columns = columns, + columns = tidyselect::all_of(columns), regex = regex, na_pass = na_pass, preconditions = preconditions, @@ -378,7 +378,7 @@ col_vals_regex <- function( assertion_type = "col_vals_regex", i_o = i_o, columns_expr = columns_expr, - column = columns[i], + column = tidyselect::all_of(columns[i]), values = regex, na_pass = na_pass, preconditions = preconditions, diff --git a/R/col_vals_within_spec.R b/R/col_vals_within_spec.R index e72167f82..7faff4abe 100644 --- a/R/col_vals_within_spec.R +++ b/R/col_vals_within_spec.R @@ -390,7 +390,7 @@ col_vals_within_spec <- function( secret_agent <- create_agent(x, label = "::QUIET::") %>% col_vals_within_spec( - columns = columns, + columns = tidyselect::all_of(columns), spec = spec, na_pass = na_pass, preconditions = preconditions, @@ -443,7 +443,7 @@ col_vals_within_spec <- function( assertion_type = "col_vals_within_spec", i_o = i_o, columns_expr = columns_expr, - column = columns[i], + column = tidyselect::all_of(columns[i]), values = spec, na_pass = na_pass, preconditions = preconditions, From 1f04ebce109dbb7dfc22d391fe748c9e2f26f318 Mon Sep 17 00:00:00 2001 From: June Choe Date: Tue, 17 Oct 2023 09:57:41 -0400 Subject: [PATCH 05/42] remove expr constraint to bring the environment along for ordinary evaluation in all_of --- R/utils.R | 3 --- 1 file changed, 3 deletions(-) diff --git a/R/utils.R b/R/utils.R index 59fc3a0c3..69552b478 100644 --- a/R/utils.R +++ b/R/utils.R @@ -231,9 +231,6 @@ resolve_columns <- function(x, var_expr, preconditions, ..., call = rlang::calle tbl <- apply_preconditions(tbl = x, preconditions = preconditions) } - # For simplicity, get the expression out of the quosure - col_expr <- rlang::quo_get_expr(var_expr) - # Revised column selection logic ## Special case `vars()`-style enquo-ing and implement backwards compatibility if (rlang::is_call(col_expr, "vars")) { From 85217be29f01d08b03c6b20095f642395e84e164 Mon Sep 17 00:00:00 2001 From: June Choe Date: Tue, 17 Oct 2023 10:02:19 -0400 Subject: [PATCH 06/42] work with vars() as expr not quosure since we don't care about the env --- R/utils.R | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/R/utils.R b/R/utils.R index 69552b478..b57344522 100644 --- a/R/utils.R +++ b/R/utils.R @@ -233,13 +233,13 @@ resolve_columns <- function(x, var_expr, preconditions, ..., call = rlang::calle # Revised column selection logic ## Special case `vars()`-style enquo-ing and implement backwards compatibility - if (rlang::is_call(col_expr, "vars")) { - cols <- rlang::call_args(col_expr) + if (rlang::is_call(rlang::quo_get_expr(var_expr), "vars")) { + cols <- rlang::call_args(var_expr) # Deparse into character vector column <- vapply(cols, rlang::as_name, character(1), USE.NAMES = FALSE) } else { ## Else, proceed with the assumption that user supplied a {tidyselect} expression - column <- tidyselect::eval_select(col_expr, tbl, error_call = call) + column <- tidyselect::eval_select(var_expr, tbl, error_call = call) column <- names(column) } From 9fcbaedad9d0235d679bd46bcb8d79b5aeab1de9 Mon Sep 17 00:00:00 2001 From: June Choe Date: Tue, 17 Oct 2023 10:09:30 -0400 Subject: [PATCH 07/42] don't use all_of() for ordinarily evaluated column argument --- R/col_vals_gt.R | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/R/col_vals_gt.R b/R/col_vals_gt.R index 09c128e82..cba1734c4 100644 --- a/R/col_vals_gt.R +++ b/R/col_vals_gt.R @@ -505,7 +505,7 @@ col_vals_gt <- function( assertion_type = "col_vals_gt", i_o = i_o, columns_expr = columns_expr, - column = tidyselect::all_of(columns[i]), + column = columns[i], values = value, na_pass = na_pass, preconditions = preconditions, From e72e49bb051bee90f4611583bd2e30eec65669be Mon Sep 17 00:00:00 2001 From: June Choe Date: Tue, 17 Oct 2023 10:42:08 -0400 Subject: [PATCH 08/42] more explicit code for resolving tbl --- R/utils.R | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/R/utils.R b/R/utils.R index b57344522..b1baa04f8 100644 --- a/R/utils.R +++ b/R/utils.R @@ -225,10 +225,15 @@ resolve_columns <- function(x, var_expr, preconditions, ..., call = rlang::calle return(character(NA_character_)) } - # Extract tbl and apply preconditions - tbl <- if (is_ptblank_agent(x)) get_tbl_object(x) else x + # Extract tbl + tbl <- if (is_ptblank_agent(x)) { + get_tbl_object(x) + } else if (is_a_table_object(x)) { + x + } + # Apply preconditions if (!is.null(preconditions)) { - tbl <- apply_preconditions(tbl = x, preconditions = preconditions) + tbl <- apply_preconditions(tbl = tbl, preconditions = preconditions) } # Revised column selection logic From ac65f404fbe7ec14f9d2575f85ad0a1d09e51934 Mon Sep 17 00:00:00 2001 From: June Choe Date: Tue, 17 Oct 2023 11:33:03 -0400 Subject: [PATCH 09/42] exception for when tbl is empty like in specially() --- R/utils.R | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/R/utils.R b/R/utils.R index b1baa04f8..aa12e639f 100644 --- a/R/utils.R +++ b/R/utils.R @@ -241,12 +241,23 @@ resolve_columns <- function(x, var_expr, preconditions, ..., call = rlang::calle if (rlang::is_call(rlang::quo_get_expr(var_expr), "vars")) { cols <- rlang::call_args(var_expr) # Deparse into character vector - column <- vapply(cols, rlang::as_name, character(1), USE.NAMES = FALSE) + col_syms <- rlang::syms(cols) + if (rlang::is_empty(tbl)) { + # Special-case `serially()` - just deparse elements and don't test for existence + column <- vapply(col_syms, rlang::as_name, character(1), USE.NAMES = FALSE) + } else { + # Convert to the idiomatic `c()`-expr + col_c_expr <- rlang::call2("c", !!!col_syms) + column <- tidyselect::eval_select(col_c_expr, tbl, error_call = call) + column <- names(column) + } + # column <- vapply(cols, rlang::as_name, character(1), USE.NAMES = FALSE) } else { ## Else, proceed with the assumption that user supplied a {tidyselect} expression column <- tidyselect::eval_select(var_expr, tbl, error_call = call) column <- names(column) } + # Just the names of the tidy-selected columns if (length(column) < 1) { column <- NA_character_ From d386432bd9b4aa22ffdb2a099ab7b28239e4d617 Mon Sep 17 00:00:00 2001 From: June Choe Date: Tue, 17 Oct 2023 11:33:36 -0400 Subject: [PATCH 10/42] use tidyselect in rows_distinct --- R/rows_distinct.R | 16 ++-------------- 1 file changed, 2 insertions(+), 14 deletions(-) diff --git a/R/rows_distinct.R b/R/rows_distinct.R index 6c822a8b3..cd1ff4dfd 100644 --- a/R/rows_distinct.R +++ b/R/rows_distinct.R @@ -280,20 +280,8 @@ rows_distinct <- function( # Capture the `columns` expression columns <- rlang::enquo(columns) - if (uses_tidyselect(expr_text = columns_expr)) { - - # Resolve the columns based on the expression - columns <- resolve_columns(x = x, var_expr = columns, preconditions = NULL) - - } else { - - # Resolve the columns based on the expression - if (!is.null(rlang::eval_tidy(columns)) && !is.null(columns)) { - columns <- resolve_columns(x = x, var_expr = columns, preconditions) - } else { - columns <- NULL - } - } + # Resolve the columns based on the expression + columns <- resolve_columns(x = x, var_expr = columns, preconditions = NULL) # Resolve segments into list segments_list <- From 1382a08773ba1de9d0a7718612f23016370a2456 Mon Sep 17 00:00:00 2001 From: June Choe Date: Tue, 17 Oct 2023 11:55:06 -0400 Subject: [PATCH 11/42] tidyselect tests first draft --- tests/testthat/test-tidyselect_integration.R | 48 ++++++++++++++++++++ 1 file changed, 48 insertions(+) create mode 100644 tests/testthat/test-tidyselect_integration.R diff --git a/tests/testthat/test-tidyselect_integration.R b/tests/testthat/test-tidyselect_integration.R new file mode 100644 index 000000000..939b10824 --- /dev/null +++ b/tests/testthat/test-tidyselect_integration.R @@ -0,0 +1,48 @@ +test_that("Full range of tidyselect features available in column selection", { + + tbl <- tibble(x = 1:2, y = 1:2, yy = 1:2, nonunique = 1) + + # Single symbol + expect_success(expect_rows_distinct(tbl, x)) + expect_failure(expect_rows_distinct(tbl, nonunique)) + + # Backward-compatibility with `vars()` syntax + expect_success(expect_rows_distinct(tbl, vars(x))) + expect_success(expect_rows_distinct(tbl, vars(x, nonunique))) + expect_failure(expect_rows_distinct(tbl, vars(nonunique))) + + # Preferred {tidyselect}-style `c()` syntax + expect_success(expect_rows_distinct(tbl, c(x))) + expect_success(expect_rows_distinct(tbl, c(x, nonunique))) + expect_failure(expect_rows_distinct(tbl, c(nonunique))) + + # {tidyselect} functions + expect_success(expect_rows_distinct(tbl, all_of("x"))) + expect_success(expect_rows_distinct(tbl, all_of(c("x", "nonunique")))) + expect_failure(expect_rows_distinct(tbl, all_of("nonunique"))) + + # NEW: {tidyselect} integer indexing + expect_success(expect_rows_distinct(tbl, 1)) + expect_success(expect_rows_distinct(tbl, c(1, 4))) + expect_failure(expect_rows_distinct(tbl, 4)) + + # NEW: {tidyselect} negative indexing + expect_success(expect_rows_distinct(tbl, -(2:4))) + expect_success(expect_rows_distinct(tbl, -(2:3))) + expect_failure(expect_rows_distinct(tbl, -(1:3))) + + # NEW: {tidyselect} functions in complex expressions + exist_col <- "y" + expect_success(expect_rows_distinct(tbl, c(x, all_of(exist_col)))) + nonexist_col <- "z" + expect_error(expect_rows_distinct(tbl, c(x, all_of(nonexist_col)))) + expect_success(expect_rows_distinct(tbl, c(x, any_of(nonexist_col)))) + + # DEPRECATION: Supplying a character vector variable still works, but signals deprecation: + options(lifecycle_verbosity = "warning") + expect_success(expect_warning( + expect_rows_distinct(tbl, exist_col), + "Using an external vector in selections was deprecated in tidyselect 1.1.0." + )) + +}) From 099ba5f0ee14f0f3ee6ac18a9470b0c91868962b Mon Sep 17 00:00:00 2001 From: June Choe Date: Tue, 17 Oct 2023 11:57:37 -0400 Subject: [PATCH 12/42] namespace tidyselect fns in test --- tests/testthat/test-tidyselect_integration.R | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/tests/testthat/test-tidyselect_integration.R b/tests/testthat/test-tidyselect_integration.R index 939b10824..2d079473d 100644 --- a/tests/testthat/test-tidyselect_integration.R +++ b/tests/testthat/test-tidyselect_integration.R @@ -1,6 +1,6 @@ test_that("Full range of tidyselect features available in column selection", { - tbl <- tibble(x = 1:2, y = 1:2, yy = 1:2, nonunique = 1) + tbl <- data.frame(x = 1:2, y = 1:2, yy = 1:2, nonunique = 1) # Single symbol expect_success(expect_rows_distinct(tbl, x)) @@ -17,9 +17,9 @@ test_that("Full range of tidyselect features available in column selection", { expect_failure(expect_rows_distinct(tbl, c(nonunique))) # {tidyselect} functions - expect_success(expect_rows_distinct(tbl, all_of("x"))) - expect_success(expect_rows_distinct(tbl, all_of(c("x", "nonunique")))) - expect_failure(expect_rows_distinct(tbl, all_of("nonunique"))) + expect_success(expect_rows_distinct(tbl, tidyselect::all_of("x"))) + expect_success(expect_rows_distinct(tbl, tidyselect::all_of(c("x", "nonunique")))) + expect_failure(expect_rows_distinct(tbl, tidyselect::all_of("nonunique"))) # NEW: {tidyselect} integer indexing expect_success(expect_rows_distinct(tbl, 1)) @@ -33,10 +33,10 @@ test_that("Full range of tidyselect features available in column selection", { # NEW: {tidyselect} functions in complex expressions exist_col <- "y" - expect_success(expect_rows_distinct(tbl, c(x, all_of(exist_col)))) + expect_success(expect_rows_distinct(tbl, c(x, tidyselect::all_of(exist_col)))) nonexist_col <- "z" - expect_error(expect_rows_distinct(tbl, c(x, all_of(nonexist_col)))) - expect_success(expect_rows_distinct(tbl, c(x, any_of(nonexist_col)))) + expect_error(expect_rows_distinct(tbl, c(x, tidyselect::all_of(nonexist_col)))) + expect_success(expect_rows_distinct(tbl, c(x, tidyselect::any_of(nonexist_col)))) # DEPRECATION: Supplying a character vector variable still works, but signals deprecation: options(lifecycle_verbosity = "warning") From 7e555582da39c146871a4ad579477528e209fd2e Mon Sep 17 00:00:00 2001 From: June Choe Date: Tue, 17 Oct 2023 14:12:04 -0400 Subject: [PATCH 13/42] revert all_of for create_validation_step, which uses ordinary evaluation --- R/col_exists.R | 2 +- R/col_is_character.R | 2 +- R/col_is_date.R | 2 +- R/col_is_factor.R | 2 +- R/col_is_integer.R | 2 +- R/col_is_logical.R | 2 +- R/col_is_numeric.R | 2 +- R/col_is_posix.R | 2 +- R/col_vals_between.R | 2 +- R/col_vals_decreasing.R | 2 +- R/col_vals_equal.R | 2 +- R/col_vals_gte.R | 2 +- R/col_vals_in_set.R | 2 +- R/col_vals_increasing.R | 2 +- R/col_vals_lt.R | 2 +- R/col_vals_lte.R | 2 +- R/col_vals_make_set.R | 2 +- R/col_vals_make_subset.R | 2 +- R/col_vals_not_between.R | 2 +- R/col_vals_not_equal.R | 2 +- R/col_vals_not_in_set.R | 2 +- R/col_vals_not_null.R | 2 +- R/col_vals_null.R | 2 +- R/col_vals_regex.R | 2 +- R/col_vals_within_spec.R | 2 +- 25 files changed, 25 insertions(+), 25 deletions(-) diff --git a/R/col_exists.R b/R/col_exists.R index 32e989ba7..058145ff6 100644 --- a/R/col_exists.R +++ b/R/col_exists.R @@ -289,7 +289,7 @@ col_exists <- function( assertion_type = "col_exists", i_o = i_o, columns_expr = columns_expr, - column = tidyselect::all_of(columns[i]), + column = columns[i], preconditions = NULL, actions = covert_actions(actions, agent), step_id = step_id[i], diff --git a/R/col_is_character.R b/R/col_is_character.R index 685926192..d08b019f3 100644 --- a/R/col_is_character.R +++ b/R/col_is_character.R @@ -282,7 +282,7 @@ col_is_character <- function( assertion_type = "col_is_character", i_o = i_o, columns_expr = columns_expr, - column = tidyselect::all_of(columns[i]), + column = columns[i], preconditions = NULL, actions = covert_actions(actions, agent), step_id = step_id[i], diff --git a/R/col_is_date.R b/R/col_is_date.R index ee3bed91a..c0b810844 100644 --- a/R/col_is_date.R +++ b/R/col_is_date.R @@ -274,7 +274,7 @@ col_is_date <- function( assertion_type = "col_is_date", i_o = i_o, columns_expr = columns_expr, - column = tidyselect::all_of(columns[i]), + column = columns[i], preconditions = NULL, actions = covert_actions(actions, agent), step_id = step_id[i], diff --git a/R/col_is_factor.R b/R/col_is_factor.R index 7d8d2e8f2..d82574971 100644 --- a/R/col_is_factor.R +++ b/R/col_is_factor.R @@ -280,7 +280,7 @@ col_is_factor <- function( assertion_type = "col_is_factor", i_o = i_o, columns_expr = columns_expr, - column = tidyselect::all_of(columns[i]), + column = columns[i], preconditions = NULL, actions = covert_actions(actions, agent), step_id = step_id[i], diff --git a/R/col_is_integer.R b/R/col_is_integer.R index f97500d1a..87e966259 100644 --- a/R/col_is_integer.R +++ b/R/col_is_integer.R @@ -278,7 +278,7 @@ col_is_integer <- function( assertion_type = "col_is_integer", i_o = i_o, columns_expr = columns_expr, - column = tidyselect::all_of(columns[i]), + column = columns[i], preconditions = NULL, actions = covert_actions(actions, agent), step_id = step_id[i], diff --git a/R/col_is_logical.R b/R/col_is_logical.R index 7ff74dfa0..695a797e9 100644 --- a/R/col_is_logical.R +++ b/R/col_is_logical.R @@ -275,7 +275,7 @@ col_is_logical <- function( assertion_type = "col_is_logical", i_o = i_o, columns_expr = columns_expr, - column = tidyselect::all_of(columns[i]), + column = columns[i], preconditions = NULL, actions = covert_actions(actions, agent), step_id = step_id[i], diff --git a/R/col_is_numeric.R b/R/col_is_numeric.R index 72f05928b..f9de9d8a0 100644 --- a/R/col_is_numeric.R +++ b/R/col_is_numeric.R @@ -275,7 +275,7 @@ col_is_numeric <- function( assertion_type = "col_is_numeric", i_o = i_o, columns_expr = columns_expr, - column = tidyselect::all_of(columns[i]), + column = columns[i], preconditions = NULL, actions = covert_actions(actions, agent), step_id = step_id[i], diff --git a/R/col_is_posix.R b/R/col_is_posix.R index dc442249c..9994ef2a3 100644 --- a/R/col_is_posix.R +++ b/R/col_is_posix.R @@ -275,7 +275,7 @@ col_is_posix <- function( assertion_type = "col_is_posix", i_o = i_o, columns_expr = columns_expr, - column = tidyselect::all_of(columns[i]), + column = columns[i], preconditions = NULL, actions = covert_actions(actions, agent), step_id = step_id[i], diff --git a/R/col_vals_between.R b/R/col_vals_between.R index f51bdc705..7b138ec97 100644 --- a/R/col_vals_between.R +++ b/R/col_vals_between.R @@ -448,7 +448,7 @@ col_vals_between <- function( assertion_type = "col_vals_between", i_o = i_o, columns_expr = columns_expr, - column = tidyselect::all_of(columns[i]), + column = columns[i], values = c(left, right), na_pass = na_pass, preconditions = preconditions, diff --git a/R/col_vals_decreasing.R b/R/col_vals_decreasing.R index 5097a5de9..6b7c21732 100644 --- a/R/col_vals_decreasing.R +++ b/R/col_vals_decreasing.R @@ -433,7 +433,7 @@ col_vals_decreasing <- function( assertion_type = "col_vals_decreasing", i_o = i_o, columns_expr = columns_expr, - column = tidyselect::all_of(columns[i]), + column = columns[i], values = stat_tol, na_pass = na_pass, preconditions = preconditions, diff --git a/R/col_vals_equal.R b/R/col_vals_equal.R index 04abb7166..9014d57dd 100644 --- a/R/col_vals_equal.R +++ b/R/col_vals_equal.R @@ -385,7 +385,7 @@ col_vals_equal <- function( assertion_type = "col_vals_equal", i_o = i_o, columns_expr = columns_expr, - column = tidyselect::all_of(columns[i]), + column = columns[i], values = value, na_pass = na_pass, preconditions = preconditions, diff --git a/R/col_vals_gte.R b/R/col_vals_gte.R index 3f2691694..eebfe8e5f 100644 --- a/R/col_vals_gte.R +++ b/R/col_vals_gte.R @@ -384,7 +384,7 @@ col_vals_gte <- function( assertion_type = "col_vals_gte", i_o = i_o, columns_expr = columns_expr, - column = tidyselect::all_of(columns[i]), + column = columns[i], values = value, na_pass = na_pass, preconditions = preconditions, diff --git a/R/col_vals_in_set.R b/R/col_vals_in_set.R index 564837179..2195f754d 100644 --- a/R/col_vals_in_set.R +++ b/R/col_vals_in_set.R @@ -377,7 +377,7 @@ col_vals_in_set <- function( assertion_type = "col_vals_in_set", i_o = i_o, columns_expr = columns_expr, - column = tidyselect::all_of(columns[i]), + column = columns[i], values = set, preconditions = preconditions, seg_expr = segments, diff --git a/R/col_vals_increasing.R b/R/col_vals_increasing.R index 521cb0db3..3c65d74e8 100644 --- a/R/col_vals_increasing.R +++ b/R/col_vals_increasing.R @@ -421,7 +421,7 @@ col_vals_increasing <- function( assertion_type = "col_vals_increasing", i_o = i_o, columns_expr = columns_expr, - column = tidyselect::all_of(columns[i]), + column = columns[i], values = stat_tol, na_pass = na_pass, preconditions = preconditions, diff --git a/R/col_vals_lt.R b/R/col_vals_lt.R index a94ab9910..d470f7b5e 100644 --- a/R/col_vals_lt.R +++ b/R/col_vals_lt.R @@ -386,7 +386,7 @@ col_vals_lt <- function( assertion_type = "col_vals_lt", i_o = i_o, columns_expr = columns_expr, - column = tidyselect::all_of(columns[i]), + column = columns[i], values = value, na_pass = na_pass, preconditions = preconditions, diff --git a/R/col_vals_lte.R b/R/col_vals_lte.R index 79f0d800c..79ae142f7 100644 --- a/R/col_vals_lte.R +++ b/R/col_vals_lte.R @@ -387,7 +387,7 @@ col_vals_lte <- function( assertion_type = "col_vals_lte", i_o = i_o, columns_expr = columns_expr, - column = tidyselect::all_of(columns[i]), + column = columns[i], values = value, na_pass = na_pass, preconditions = preconditions, diff --git a/R/col_vals_make_set.R b/R/col_vals_make_set.R index 81bde081b..3eb28afde 100644 --- a/R/col_vals_make_set.R +++ b/R/col_vals_make_set.R @@ -379,7 +379,7 @@ col_vals_make_set <- function( assertion_type = "col_vals_make_set", i_o = i_o, columns_expr = columns_expr, - column = tidyselect::all_of(columns[i]), + column = columns[i], values = set, preconditions = preconditions, seg_expr = segments, diff --git a/R/col_vals_make_subset.R b/R/col_vals_make_subset.R index 49ecfc234..b039b68af 100644 --- a/R/col_vals_make_subset.R +++ b/R/col_vals_make_subset.R @@ -376,7 +376,7 @@ col_vals_make_subset <- function( assertion_type = "col_vals_make_subset", i_o = i_o, columns_expr = columns_expr, - column = tidyselect::all_of(columns[i]), + column = columns[i], values = set, preconditions = preconditions, seg_expr = segments, diff --git a/R/col_vals_not_between.R b/R/col_vals_not_between.R index 96ea45941..e167b46bc 100644 --- a/R/col_vals_not_between.R +++ b/R/col_vals_not_between.R @@ -451,7 +451,7 @@ col_vals_not_between <- function( assertion_type = "col_vals_not_between", i_o = i_o, columns_expr = columns_expr, - column = tidyselect::all_of(columns[i]), + column = columns[i], values = c(left, right), na_pass = na_pass, preconditions = preconditions, diff --git a/R/col_vals_not_equal.R b/R/col_vals_not_equal.R index 5436dc1cd..78ddaf057 100644 --- a/R/col_vals_not_equal.R +++ b/R/col_vals_not_equal.R @@ -384,7 +384,7 @@ col_vals_not_equal <- function( assertion_type = "col_vals_not_equal", i_o = i_o, columns_expr = columns_expr, - column = tidyselect::all_of(columns[i]), + column = columns[i], values = value, na_pass = na_pass, preconditions = preconditions, diff --git a/R/col_vals_not_in_set.R b/R/col_vals_not_in_set.R index 1f4ea2e23..31b975754 100644 --- a/R/col_vals_not_in_set.R +++ b/R/col_vals_not_in_set.R @@ -373,7 +373,7 @@ col_vals_not_in_set <- function( assertion_type = "col_vals_not_in_set", i_o = i_o, columns_expr = columns_expr, - column = tidyselect::all_of(columns[i]), + column = columns[i], values = set, preconditions = preconditions, seg_expr = segments, diff --git a/R/col_vals_not_null.R b/R/col_vals_not_null.R index 90e7c7d59..75b743ccd 100644 --- a/R/col_vals_not_null.R +++ b/R/col_vals_not_null.R @@ -363,7 +363,7 @@ col_vals_not_null <- function( assertion_type = "col_vals_not_null", i_o = i_o, columns_expr = columns_expr, - column = tidyselect::all_of(columns[i]), + column = columns[i], preconditions = preconditions, seg_expr = segments, seg_col = seg_col, diff --git a/R/col_vals_null.R b/R/col_vals_null.R index d18b93d57..e36f99da0 100644 --- a/R/col_vals_null.R +++ b/R/col_vals_null.R @@ -362,7 +362,7 @@ col_vals_null <- function( assertion_type = "col_vals_null", i_o = i_o, columns_expr = columns_expr, - column = tidyselect::all_of(columns[i]), + column = columns[i], preconditions = preconditions, seg_expr = segments, seg_col = seg_col, diff --git a/R/col_vals_regex.R b/R/col_vals_regex.R index 9f49384e8..cbd899c63 100644 --- a/R/col_vals_regex.R +++ b/R/col_vals_regex.R @@ -378,7 +378,7 @@ col_vals_regex <- function( assertion_type = "col_vals_regex", i_o = i_o, columns_expr = columns_expr, - column = tidyselect::all_of(columns[i]), + column = columns[i], values = regex, na_pass = na_pass, preconditions = preconditions, diff --git a/R/col_vals_within_spec.R b/R/col_vals_within_spec.R index 7faff4abe..d22cef116 100644 --- a/R/col_vals_within_spec.R +++ b/R/col_vals_within_spec.R @@ -443,7 +443,7 @@ col_vals_within_spec <- function( assertion_type = "col_vals_within_spec", i_o = i_o, columns_expr = columns_expr, - column = tidyselect::all_of(columns[i]), + column = columns[i], values = spec, na_pass = na_pass, preconditions = preconditions, From 14f9878159ba0b5a4442e618828e50c81aeccf89 Mon Sep 17 00:00:00 2001 From: June Choe Date: Tue, 17 Oct 2023 14:23:13 -0400 Subject: [PATCH 14/42] use all_of --- R/rows_distinct.R | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/R/rows_distinct.R b/R/rows_distinct.R index cd1ff4dfd..36502e5c2 100644 --- a/R/rows_distinct.R +++ b/R/rows_distinct.R @@ -296,7 +296,7 @@ rows_distinct <- function( secret_agent <- create_agent(x, label = "::QUIET::") %>% rows_distinct( - columns = columns, + columns = tidyselect::all_of(columns), preconditions = preconditions, segments = segments, label = label, From 4cc1c8425d959d7dcc9bea34fa12332053862e4f Mon Sep 17 00:00:00 2001 From: June Choe Date: Tue, 17 Oct 2023 14:32:14 -0400 Subject: [PATCH 15/42] implement behavior of NULL with everything() --- R/rows_distinct.R | 3 +++ tests/testthat/test-tidyselect_integration.R | 5 +++++ 2 files changed, 8 insertions(+) diff --git a/R/rows_distinct.R b/R/rows_distinct.R index 36502e5c2..9cb278fc4 100644 --- a/R/rows_distinct.R +++ b/R/rows_distinct.R @@ -279,6 +279,9 @@ rows_distinct <- function( # Capture the `columns` expression columns <- rlang::enquo(columns) + if (is.null(rlang::quo_get_expr(columns))) { + columns <- rlang::new_quosure(rlang::expr(tidyselect::everything())) + } # Resolve the columns based on the expression columns <- resolve_columns(x = x, var_expr = columns, preconditions = NULL) diff --git a/tests/testthat/test-tidyselect_integration.R b/tests/testthat/test-tidyselect_integration.R index 2d079473d..53a9128e4 100644 --- a/tests/testthat/test-tidyselect_integration.R +++ b/tests/testthat/test-tidyselect_integration.R @@ -45,4 +45,9 @@ test_that("Full range of tidyselect features available in column selection", { "Using an external vector in selections was deprecated in tidyselect 1.1.0." )) + # For `rows_distinct()` specifically, NULL = "select everything" behavior implemented in tidyselect: + expect_success(expect_rows_distinct(tbl, )) + tbl_only_nonunique <- tbl[,"nonunique", drop = FALSE] + expect_failure(expect_rows_distinct(tbl_only_nonunique, )) + }) From 91d3102b38d25e33f10d6eb044224f169c9d0621 Mon Sep 17 00:00:00 2001 From: June Choe Date: Tue, 17 Oct 2023 14:39:34 -0400 Subject: [PATCH 16/42] relax regexp for column not found error --- tests/testthat/test-test_fns.R | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/tests/testthat/test-test_fns.R b/tests/testthat/test-test_fns.R index ef2d9fd19..b54d36578 100644 --- a/tests/testthat/test-test_fns.R +++ b/tests/testthat/test-test_fns.R @@ -787,7 +787,8 @@ test_that("pointblank expectation functions produce the correct results", { test_that("expect errors to be expressed by pointblank under some conditions", { - no_col_msg <- "The value for `column` doesn't correspond to a column name." + # no_col_msg <- "The value for `column` doesn't correspond to a column name." + no_col_msg <- "column" # Errors caught and expressed when a column doesn't exist expect_error(test_col_vals_lt(tbl, columns = vars(z), value = 0), regexp = no_col_msg) From d7c1eff275ba81c381490f96e6d56b8e9613a492 Mon Sep 17 00:00:00 2001 From: June Choe Date: Tue, 17 Oct 2023 15:09:57 -0400 Subject: [PATCH 17/42] tests expect column info from row_distinct() validation step --- tests/testthat/test-create_validation_steps.R | 2 +- tests/testthat/test-interrogate_with_agent.R | 4 ++-- tests/testthat/test-interrogate_with_agent_db.R | 4 ++-- tests/testthat/test-yaml.R | 2 +- 4 files changed, 6 insertions(+), 6 deletions(-) diff --git a/tests/testthat/test-create_validation_steps.R b/tests/testthat/test-create_validation_steps.R index d0e0777ce..3a4318de6 100644 --- a/tests/testthat/test-create_validation_steps.R +++ b/tests/testthat/test-create_validation_steps.R @@ -1036,7 +1036,7 @@ test_that("Creating a `rows_distinct()` step is possible", { expect_equivalent(validation$tbl_name, "small_table") expect_equivalent(validation$col_names, c("date_time", "date", "a", "b", "c", "d", "e", "f")) expect_equivalent(validation$validation_set$assertion_type, "rows_distinct") - expect_true(is.na(validation$validation_set$column %>% .[[1]] %>% .[[1]])) + expect_equivalent(validation$validation_set$column %>% unlist(), "date_time, date, a, b, c, d, e, f") expect_true(is.null(validation$validation_set[["values"]][[1]])) expect_true(is.na(validation$validation_set$all_passed)) expect_true(is.na(validation$validation_set$n)) diff --git a/tests/testthat/test-interrogate_with_agent.R b/tests/testthat/test-interrogate_with_agent.R index eee9c0bfa..4cc70dfed 100644 --- a/tests/testthat/test-interrogate_with_agent.R +++ b/tests/testthat/test-interrogate_with_agent.R @@ -784,7 +784,7 @@ test_that("Interrogating for valid row values", { # Expect certain values in `validation$validation_set` expect_equivalent(validation$tbl_name, "small_table") expect_equivalent(validation$validation_set$assertion_type, "rows_distinct") - expect_true(is.na(validation$validation_set$column %>% unlist())) + expect_equivalent(validation$validation_set$column %>% unlist(), "date_time, date, a, b, c, d, e, f") expect_true(is.null(validation$validation_set[["values"]][[1]])) expect_false(validation$validation_set$all_passed) expect_equivalent(validation$validation_set$n, 13) @@ -807,7 +807,7 @@ test_that("Interrogating for valid row values", { # Expect certain values in `validation$validation_set` expect_equivalent(validation$tbl_name, "small_table") expect_equivalent(validation$validation_set$assertion_type, "rows_distinct") - expect_true(is.na(validation$validation_set$column %>% unlist())) + expect_equivalent(validation$validation_set$column %>% unlist(), "date_time, date, a, b, c, d, e, f") expect_true(is.null(validation$validation_set[["values"]][[1]])) expect_true(validation$validation_set$all_passed) expect_equivalent(validation$validation_set$n, 11) diff --git a/tests/testthat/test-interrogate_with_agent_db.R b/tests/testthat/test-interrogate_with_agent_db.R index 8d8d24750..cf8b57ee5 100644 --- a/tests/testthat/test-interrogate_with_agent_db.R +++ b/tests/testthat/test-interrogate_with_agent_db.R @@ -485,7 +485,7 @@ test_that("Interrogating for valid row values", { # Expect certain values in `validation$validation_set` expect_equivalent(validation$tbl_name, "small_table") expect_equivalent(validation$validation_set$assertion_type, "rows_distinct") - expect_true(is.na(validation$validation_set$column %>% unlist())) + expect_equivalent(validation$validation_set$column %>% unlist(), "date_time, date, a, b, c, d, e, f") expect_true(is.null(validation$validation_set[["values"]][[1]])) expect_false(validation$validation_set$all_passed) expect_equivalent(validation$validation_set$n, 13) @@ -510,7 +510,7 @@ test_that("Interrogating for valid row values", { # Expect certain values in `validation$validation_set` expect_equivalent(validation$tbl_name, "small_table") expect_equivalent(validation$validation_set$assertion_type, "rows_distinct") - expect_true(is.na(validation$validation_set$column %>% unlist())) + expect_equivalent(validation$validation_set$column %>% unlist(), "date_time, date, a, b, c, d, e, f") expect_true(is.null(validation$validation_set[["values"]][[1]])) expect_true(validation$validation_set$all_passed) expect_equivalent(validation$validation_set$n, 11) diff --git a/tests/testthat/test-yaml.R b/tests/testthat/test-yaml.R index 7c0447f9b..02279b826 100644 --- a/tests/testthat/test-yaml.R +++ b/tests/testthat/test-yaml.R @@ -800,7 +800,7 @@ test_that("Individual validation steps make the YAML round-trip successfully", { expect_equal( get_oneline_expr_str(agent %>% rows_distinct()), - "rows_distinct()" + "rows_distinct(columns = vars(date_time, date, a, b, c, d, e, f))" ) expect_equal( get_oneline_expr_str(agent %>% rows_distinct(columns = vars(a, b))), From 7b3ee392a5c9213be9c0c1970392920b5a67651a Mon Sep 17 00:00:00 2001 From: June Choe Date: Tue, 17 Oct 2023 15:55:01 -0400 Subject: [PATCH 18/42] add test for tidyselect where support --- tests/testthat/test-tidyselect_integration.R | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/tests/testthat/test-tidyselect_integration.R b/tests/testthat/test-tidyselect_integration.R index 53a9128e4..b141eda76 100644 --- a/tests/testthat/test-tidyselect_integration.R +++ b/tests/testthat/test-tidyselect_integration.R @@ -1,6 +1,6 @@ test_that("Full range of tidyselect features available in column selection", { - tbl <- data.frame(x = 1:2, y = 1:2, yy = 1:2, nonunique = 1) + tbl <- data.frame(x = 1:2, y = 1:2, yy = 1:2, nonunique = "A") # Single symbol expect_success(expect_rows_distinct(tbl, x)) @@ -31,6 +31,11 @@ test_that("Full range of tidyselect features available in column selection", { expect_success(expect_rows_distinct(tbl, -(2:3))) expect_failure(expect_rows_distinct(tbl, -(1:3))) + # NEW: {tidyselect} `where()` predicate: + expect_success(expect_rows_distinct(tbl, !tidyselect::where(is.character))) + expect_success(expect_rows_distinct(tbl, tidyselect::where(is.numeric))) + expect_failure(expect_rows_distinct(tbl, tidyselect::where(is.character))) + # NEW: {tidyselect} functions in complex expressions exist_col <- "y" expect_success(expect_rows_distinct(tbl, c(x, tidyselect::all_of(exist_col)))) From 9bc5524cccaf45cd5299af5a2dc11db2c855c20c Mon Sep 17 00:00:00 2001 From: June Choe Date: Tue, 17 Oct 2023 16:52:20 -0400 Subject: [PATCH 19/42] clean up resolve_columns --- R/utils.R | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/R/utils.R b/R/utils.R index aa12e639f..1a1f6e566 100644 --- a/R/utils.R +++ b/R/utils.R @@ -221,7 +221,7 @@ materialize_table <- function(tbl, check = TRUE) { resolve_columns <- function(x, var_expr, preconditions, ..., call = rlang::caller_env()) { # Return an empty character vector if the expr is NULL - if (is.null(var_expr)) { + if (is.null(rlang::quo_get_expr(var_expr))) { return(character(NA_character_)) } @@ -251,13 +251,11 @@ resolve_columns <- function(x, var_expr, preconditions, ..., call = rlang::calle column <- tidyselect::eval_select(col_c_expr, tbl, error_call = call) column <- names(column) } - # column <- vapply(cols, rlang::as_name, character(1), USE.NAMES = FALSE) } else { ## Else, proceed with the assumption that user supplied a {tidyselect} expression column <- tidyselect::eval_select(var_expr, tbl, error_call = call) column <- names(column) } - # Just the names of the tidy-selected columns if (length(column) < 1) { column <- NA_character_ From 982d4c5a2a49a117b72ed97bd30815e674f770c9 Mon Sep 17 00:00:00 2001 From: June Choe Date: Tue, 17 Oct 2023 17:19:25 -0400 Subject: [PATCH 20/42] use quo_is_null --- R/utils.R | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/R/utils.R b/R/utils.R index 1a1f6e566..66401c810 100644 --- a/R/utils.R +++ b/R/utils.R @@ -221,7 +221,7 @@ materialize_table <- function(tbl, check = TRUE) { resolve_columns <- function(x, var_expr, preconditions, ..., call = rlang::caller_env()) { # Return an empty character vector if the expr is NULL - if (is.null(rlang::quo_get_expr(var_expr))) { + if (rlang::quo_is_null(var_expr)) { return(character(NA_character_)) } From 719cf6f618b6b71e449cc5db6bde84bf25021545 Mon Sep 17 00:00:00 2001 From: June Choe Date: Tue, 17 Oct 2023 17:23:06 -0400 Subject: [PATCH 21/42] use quo_is_call --- R/utils.R | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/R/utils.R b/R/utils.R index 66401c810..830343f72 100644 --- a/R/utils.R +++ b/R/utils.R @@ -238,9 +238,9 @@ resolve_columns <- function(x, var_expr, preconditions, ..., call = rlang::calle # Revised column selection logic ## Special case `vars()`-style enquo-ing and implement backwards compatibility - if (rlang::is_call(rlang::quo_get_expr(var_expr), "vars")) { + if (rlang::quo_is_call(var_expr, "vars")) { cols <- rlang::call_args(var_expr) - # Deparse into character vector + # Ensure elements are symbols col_syms <- rlang::syms(cols) if (rlang::is_empty(tbl)) { # Special-case `serially()` - just deparse elements and don't test for existence From 5bbe10113090244a47df64bf93114f839d3ecd7d Mon Sep 17 00:00:00 2001 From: June Choe Date: Wed, 18 Oct 2023 12:03:06 -0400 Subject: [PATCH 22/42] simplify defaulting to everything() when column is NULL for rows_distinct() --- R/rows_distinct.R | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/R/rows_distinct.R b/R/rows_distinct.R index 9cb278fc4..c06299f07 100644 --- a/R/rows_distinct.R +++ b/R/rows_distinct.R @@ -279,8 +279,8 @@ rows_distinct <- function( # Capture the `columns` expression columns <- rlang::enquo(columns) - if (is.null(rlang::quo_get_expr(columns))) { - columns <- rlang::new_quosure(rlang::expr(tidyselect::everything())) + if (rlang::quo_is_null(columns)) { + columns <- rlang::quo(tidyselect::everything()) } # Resolve the columns based on the expression From 646ec8e9f3e4adeeafa53914dca11a75a8a91b1a Mon Sep 17 00:00:00 2001 From: June Choe Date: Thu, 19 Oct 2023 10:11:00 -0400 Subject: [PATCH 23/42] [chore] probe_columns helpers marked for always receiving chr input --- R/scan_data.R | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/R/scan_data.R b/R/scan_data.R index db2cb3a32..08ba919ca 100644 --- a/R/scan_data.R +++ b/R/scan_data.R @@ -1041,7 +1041,7 @@ probe_columns_numeric <- function( locale ) { - data_column <- dplyr::select(data, {{ column }}) + data_column <- dplyr::select(data, tidyselect::any_of(column)) column_description_gt <- get_column_description_gt( @@ -1133,7 +1133,7 @@ probe_columns_character <- function( locale ) { - data_column <- data %>% dplyr::select({{ column }}) + data_column <- data %>% dplyr::select(tidyselect::any_of(column)) column_description_gt <- get_column_description_gt( @@ -1184,7 +1184,7 @@ probe_columns_logical <- function( locale ) { - data_column <- data %>% dplyr::select({{ column }}) + data_column <- data %>% dplyr::select(tidyselect::any_of(column)) column_description_gt <- get_column_description_gt( @@ -1211,7 +1211,7 @@ probe_columns_factor <- function( locale ) { - data_column <- data %>% dplyr::select({{ column }}) + data_column <- data %>% dplyr::select(tidyselect::any_of(column)) column_description_gt <- get_column_description_gt( @@ -1238,7 +1238,7 @@ probe_columns_date <- function( locale ) { - data_column <- data %>% dplyr::select({{ column }}) + data_column <- data %>% dplyr::select(tidyselect::any_of(column)) column_description_gt <- get_column_description_gt( @@ -1265,7 +1265,7 @@ probe_columns_posix <- function( locale ) { - data_column <- data %>% dplyr::select({{ column }}) + data_column <- data %>% dplyr::select(tidyselect::any_of(column)) column_description_gt <- get_column_description_gt( @@ -1290,7 +1290,7 @@ probe_columns_other <- function( n_rows ) { - data_column <- data %>% dplyr::select({{ column }}) + data_column <- data %>% dplyr::select(tidyselect::any_of(column)) column_classes <- paste(class(data_column), collapse = ", ") From 1978e8d137a9e7938b8602f4f2270fae2b13b1bb Mon Sep 17 00:00:00 2001 From: June Choe Date: Thu, 19 Oct 2023 10:47:14 -0400 Subject: [PATCH 24/42] [chore] some more all_of coverage --- R/table_transformers.R | 10 +++++----- R/utils.R | 2 +- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/R/table_transformers.R b/R/table_transformers.R index 66cd2dce7..a38bb0fec 100644 --- a/R/table_transformers.R +++ b/R/table_transformers.R @@ -161,7 +161,7 @@ tt_summary_stats <- function(tbl) { if (r_col_types[i] %in% c("integer", "numeric")) { - data_col <- dplyr::select(tbl, col_names[i]) + data_col <- dplyr::select(tbl, tidyselect::all_of(col_names[i])) # nocov start @@ -286,7 +286,7 @@ tt_string_info <- function(tbl) { if (r_col_types[i] == "character") { - data_col <- dplyr::select(tbl, col_names[i]) + data_col <- dplyr::select(tbl, tidyselect::all_of(col_names[i])) suppressWarnings({ info_list <- get_table_column_nchar_stats(data_column = data_col) @@ -585,7 +585,7 @@ tt_time_shift <- function( tbl %>% dplyr::mutate( dplyr::across( - .cols = time_columns, + .cols = tidyselect::all_of(time_columns), .fns = ~ lubridate::days(n_days) + . ) ) @@ -596,7 +596,7 @@ tt_time_shift <- function( tbl %>% dplyr::mutate( dplyr::across( - .cols = time_columns, + .cols = tidyselect::all_of(time_columns), .fns = ~ time_shift + . ) ) @@ -645,7 +645,7 @@ tt_time_shift <- function( tbl %>% dplyr::mutate( dplyr::across( - .cols = time_columns, + .cols = tidyselect::all_of(time_columns), .fns = ~ fn_time(time_value * direction_val) + .) ) } diff --git a/R/utils.R b/R/utils.R index 830343f72..35ade7487 100644 --- a/R/utils.R +++ b/R/utils.R @@ -902,7 +902,7 @@ get_tbl_information_dbi <- function(tbl) { DBI::dbDataType( tbl_connection, tbl %>% - dplyr::select(x) %>% + dplyr::select(tidyselect::all_of(x)) %>% utils::head(1) %>% dplyr::collect() %>% dplyr::pull(x) From 65ae1adc8dc43034a5c4a98fead0bafa66fe43b8 Mon Sep 17 00:00:00 2001 From: June Choe Date: Thu, 19 Oct 2023 14:57:48 -0400 Subject: [PATCH 25/42] linter --- R/utils.R | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/R/utils.R b/R/utils.R index 35ade7487..85d4a9b27 100644 --- a/R/utils.R +++ b/R/utils.R @@ -218,7 +218,8 @@ materialize_table <- function(tbl, check = TRUE) { tbl } -resolve_columns <- function(x, var_expr, preconditions, ..., call = rlang::caller_env()) { +resolve_columns <- function(x, var_expr, preconditions, ..., + call = rlang::caller_env()) { # Return an empty character vector if the expr is NULL if (rlang::quo_is_null(var_expr)) { @@ -237,14 +238,15 @@ resolve_columns <- function(x, var_expr, preconditions, ..., call = rlang::calle } # Revised column selection logic - ## Special case `vars()`-style enquo-ing and implement backwards compatibility + ## Special case `vars()`-expression for backwards compatibility if (rlang::quo_is_call(var_expr, "vars")) { cols <- rlang::call_args(var_expr) # Ensure elements are symbols col_syms <- rlang::syms(cols) if (rlang::is_empty(tbl)) { - # Special-case `serially()` - just deparse elements and don't test for existence - column <- vapply(col_syms, rlang::as_name, character(1), USE.NAMES = FALSE) + # Special-case `serially()` - just deparse elements and bypass tidyselect + column <- vapply(col_syms, rlang::as_name, character(1), + USE.NAMES = FALSE) } else { # Convert to the idiomatic `c()`-expr col_c_expr <- rlang::call2("c", !!!col_syms) @@ -252,7 +254,7 @@ resolve_columns <- function(x, var_expr, preconditions, ..., call = rlang::calle column <- names(column) } } else { - ## Else, proceed with the assumption that user supplied a {tidyselect} expression + ## Else, assume that the user supplied a valid tidyselect expression column <- tidyselect::eval_select(var_expr, tbl, error_call = call) column <- names(column) } From a50643dc3b539d0e6b59335a2847ee5467af0dc3 Mon Sep 17 00:00:00 2001 From: June Choe Date: Thu, 19 Oct 2023 18:38:51 -0400 Subject: [PATCH 26/42] tidyselect errors only in non-validation-building contexts --- R/utils.R | 38 ++++++++++++++++++++++++++++++-------- 1 file changed, 30 insertions(+), 8 deletions(-) diff --git a/R/utils.R b/R/utils.R index 85d4a9b27..40ddd3b6e 100644 --- a/R/utils.R +++ b/R/utils.R @@ -218,8 +218,32 @@ materialize_table <- function(tbl, check = TRUE) { tbl } -resolve_columns <- function(x, var_expr, preconditions, ..., - call = rlang::caller_env()) { +is_secret_agent <- function(x) { + is_ptblank_agent(x) && (x$label == "::QUIET::") +} + +resolve_columns <- function(x, var_expr, preconditions) { + + out <- tryCatch( + expr = resolve_columns_internal(x, var_expr, preconditions), + error = function(cnd) cnd + ) + + if (rlang::is_error(out)) { + # If not in validation-planning context (assert/expect/test) + if (is_a_table_object(x) || is_secret_agent(x)) { + rlang::cnd_signal(out) + } else { + # Else (if building up validations): return columns attempted to subset + out$i + } + } else { + out + } + +} + +resolve_columns_internal <- function(x, var_expr, preconditions) { # Return an empty character vector if the expr is NULL if (rlang::quo_is_null(var_expr)) { @@ -241,21 +265,19 @@ resolve_columns <- function(x, var_expr, preconditions, ..., ## Special case `vars()`-expression for backwards compatibility if (rlang::quo_is_call(var_expr, "vars")) { cols <- rlang::call_args(var_expr) - # Ensure elements are symbols - col_syms <- rlang::syms(cols) if (rlang::is_empty(tbl)) { # Special-case `serially()` - just deparse elements and bypass tidyselect - column <- vapply(col_syms, rlang::as_name, character(1), + column <- vapply(cols, rlang::as_name, character(1), USE.NAMES = FALSE) } else { # Convert to the idiomatic `c()`-expr - col_c_expr <- rlang::call2("c", !!!col_syms) - column <- tidyselect::eval_select(col_c_expr, tbl, error_call = call) + col_c_expr <- rlang::call2("c", !!!cols) + column <- tidyselect::eval_select(col_c_expr, tbl) column <- names(column) } } else { ## Else, assume that the user supplied a valid tidyselect expression - column <- tidyselect::eval_select(var_expr, tbl, error_call = call) + column <- tidyselect::eval_select(var_expr, tbl) column <- names(column) } From 35ae592a2ff32602b1ca67cb4e0e4ca609832487 Mon Sep 17 00:00:00 2001 From: June Choe Date: Thu, 19 Oct 2023 19:22:17 -0400 Subject: [PATCH 27/42] missing columns for selection marked as NA --- R/utils.R | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/R/utils.R b/R/utils.R index 40ddd3b6e..e9b3cab2a 100644 --- a/R/utils.R +++ b/R/utils.R @@ -235,7 +235,8 @@ resolve_columns <- function(x, var_expr, preconditions) { rlang::cnd_signal(out) } else { # Else (if building up validations): return columns attempted to subset - out$i + # or NA if none + out$i %||% NA_character_ } } else { out From dddfd1e430be80ab33abe2f81e9fe0353d5c4026 Mon Sep 17 00:00:00 2001 From: June Choe Date: Thu, 19 Oct 2023 19:50:52 -0400 Subject: [PATCH 28/42] Resolve to NA for when NULL passed to columns argument --- R/utils.R | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/R/utils.R b/R/utils.R index e9b3cab2a..ddd825d5c 100644 --- a/R/utils.R +++ b/R/utils.R @@ -246,9 +246,9 @@ resolve_columns <- function(x, var_expr, preconditions) { resolve_columns_internal <- function(x, var_expr, preconditions) { - # Return an empty character vector if the expr is NULL + # Return NA if the expr is NULL if (rlang::quo_is_null(var_expr)) { - return(character(NA_character_)) + return(NA_character_) } # Extract tbl From af394ab681d29a70f8daa26ed57a12a2cab30193 Mon Sep 17 00:00:00 2001 From: June Choe Date: Thu, 19 Oct 2023 20:53:39 -0400 Subject: [PATCH 29/42] force tbl/agent in resolve_columns to avoid interrupt promise warnings --- R/utils.R | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/R/utils.R b/R/utils.R index ddd825d5c..3d85d8e6b 100644 --- a/R/utils.R +++ b/R/utils.R @@ -224,6 +224,8 @@ is_secret_agent <- function(x) { resolve_columns <- function(x, var_expr, preconditions) { + force(x) # To avoid `restarting interrupted promise evaluation` warnings + out <- tryCatch( expr = resolve_columns_internal(x, var_expr, preconditions), error = function(cnd) cnd @@ -234,8 +236,7 @@ resolve_columns <- function(x, var_expr, preconditions) { if (is_a_table_object(x) || is_secret_agent(x)) { rlang::cnd_signal(out) } else { - # Else (if building up validations): return columns attempted to subset - # or NA if none + # Else (mid-planning): return columns attempted to subset or NA if empty out$i %||% NA_character_ } } else { From 7111ed25842fa1d935c73a77491211e33cc2f651 Mon Sep 17 00:00:00 2001 From: June Choe Date: Thu, 19 Oct 2023 21:20:31 -0400 Subject: [PATCH 30/42] tests for tidyselect column selection error behaviors --- tests/testthat/test-tidyselect_fails_safely.R | 118 ++++++++++++++++++ 1 file changed, 118 insertions(+) create mode 100644 tests/testthat/test-tidyselect_fails_safely.R diff --git a/tests/testthat/test-tidyselect_fails_safely.R b/tests/testthat/test-tidyselect_fails_safely.R new file mode 100644 index 000000000..d6e8c9745 --- /dev/null +++ b/tests/testthat/test-tidyselect_fails_safely.R @@ -0,0 +1,118 @@ +agent <- create_agent(small_table) +nonexistent_col <- "z" + +test_that("tidyselect errors signaled at report, not during development of validation plan", { + + # No immediate error for all patterns + expect_s3_class(a1 <- agent %>% col_vals_not_null(z), "ptblank_agent") + expect_s3_class(a2 <- agent %>% col_vals_not_null("z"), "ptblank_agent") + expect_s3_class(a3 <- agent %>% col_vals_not_null(all_of("z")), "ptblank_agent") + expect_s3_class(a4 <- agent %>% col_vals_not_null(all_of(nonexistent_col)), "ptblank_agent") + + # Failure signaled via report + expect_message(expect_no_error(a1 %>% interrogate()), "ERROR") + expect_message(expect_no_error(a2 %>% interrogate()), "ERROR") + expect_message(expect_no_error(a3 %>% interrogate()), "ERROR") + expect_message(expect_no_error(a4 %>% interrogate()), "ERROR") + + # Stress testing + expect_no_error(agent %>% col_vals_not_null(stop())) + expect_no_error(agent %>% col_vals_not_null(c(stop()))) + expect_no_error(agent %>% col_vals_not_null(all_of(stop()))) + +}) + +test_that("fail state correctly registered in the report for tidyselect errors", { + + # Adopted from test-get_agent_report.R ------------------------- + + # The following agent will perform an interrogation that results + # in all test units passing in the second validation step, but + # the first experiences an evaluation error (since column + # `z` doesn't exist in `small_table`) + agent <- + create_agent(tbl = small_table) %>% + col_vals_not_null(all_of("z")) %>% # swapped for `vars("z")` + col_vals_gt(vars(c), 1, na_pass = TRUE) %>% + interrogate() + + # Expect that the interrogation *does not* have + # a completely 'all passed' state (returning FALSE) + agent %>% all_passed() %>% expect_false() + + # If narrowing the `all_passed()` evaluation to only + # the second validation step, then we should expect TRUE + agent %>% all_passed(i = 2) %>% expect_true() + + # If narrowing the `all_passed()` evaluation to only + # the first validation step, then we should expect FALSE + agent %>% all_passed(i = 1) %>% expect_false() + +}) + +test_that("(tidy-)selecting 0 columns = skip the validation step at interrogation", { + + # Old behavior for vars()/NULL/ preserved: + ## 1) No immediate error for zero columns selected + expect_s3_class(a5 <- agent %>% col_vals_not_null(vars()), "ptblank_agent") + expect_s3_class(a6 <- agent %>% col_vals_not_null(NULL), "ptblank_agent") + expect_s3_class(a7 <- agent %>% col_vals_not_null(), "ptblank_agent") + ## 2) # Treated as inactive in the report + expect_message(expect_no_error(a5 %>% interrogate()), "Skipping") + expect_message(expect_no_error(a6 %>% interrogate()), "Skipping") + expect_message(expect_no_error(a7 %>% interrogate()), "Skipping") + + # Same behavior of 0-column selection replicated in tidyselect patterns + expect_length(small_table %>% dplyr::select(any_of("z")), 0) + expect_length(small_table %>% dplyr::select(c()), 0) + expect_s3_class(a8 <- agent %>% col_vals_not_null(any_of("z")), "ptblank_agent") + expect_s3_class(a9 <- agent %>% col_vals_not_null(c()), "ptblank_agent") + expect_message(expect_no_error(a8 %>% interrogate()), "Skipping") + expect_message(expect_no_error(a9 %>% interrogate()), "Skipping") + +}) + +test_that("tidyselect errors *are* immediate for assertion/expectation/test", { + + mismatch_msg <- "Can't subset columns that don't exist." + + # For validation steps are used on table + expect_error(small_table %>% col_vals_not_null(z), mismatch_msg) + expect_error(small_table %>% col_vals_not_null("z"), mismatch_msg) + expect_error(small_table %>% col_vals_not_null(all_of("z")), mismatch_msg) + expect_error(small_table %>% col_vals_not_null(all_of(nonexistent_col)), mismatch_msg) + + # For expectations + expect_error(small_table %>% expect_col_vals_not_null(z), mismatch_msg) + expect_error(small_table %>% expect_col_vals_not_null("z"), mismatch_msg) + expect_error(small_table %>% expect_col_vals_not_null(all_of("z")), mismatch_msg) + expect_error(small_table %>% expect_col_vals_not_null(all_of(nonexistent_col)), mismatch_msg) + + # For tests + expect_error(small_table %>% test_col_vals_not_null(z), mismatch_msg) + expect_error(small_table %>% test_col_vals_not_null("z"), mismatch_msg) + expect_error(small_table %>% test_col_vals_not_null(all_of("z")), mismatch_msg) + expect_error(small_table %>% test_col_vals_not_null(all_of(nonexistent_col)), mismatch_msg) + +}) + +test_that("tidyselect errors cannot be downgraded in assertion/expectation on table", { + + # This replicates old behavior + expect_error({ + small_table %>% + col_vals_not_null(a) %>% + col_vals_not_null(z, actions = warn_on_fail()) %>% + col_vals_not_null(b) + }, "Can't subset columns that don't exist.") + +}) + +# test_that("Other things that may need to get ironed out", { +# # Empty `vars()` in rows_* functions resolve to `list(NA)` instead of `NA` +# agent %>% rows_distinct(vars()) +# agent %>% rows_complete(vars()) +# # Attempting to select using a non-existent variable silently fails +# agent %>% col_vals_not_null(all_of(nonexistent_var)) %>% interrogate() +# # Current heuristic for re-throwing the error relies on whether agent is "::QUIET::" ... +# }) From 726aed6e27381d1ef91f7a215df7358b60b2b042 Mon Sep 17 00:00:00 2001 From: June Choe Date: Thu, 19 Oct 2023 22:22:43 -0400 Subject: [PATCH 31/42] test status in validation set directly --- tests/testthat/test-tidyselect_fails_safely.R | 20 ++++++++++--------- 1 file changed, 11 insertions(+), 9 deletions(-) diff --git a/tests/testthat/test-tidyselect_fails_safely.R b/tests/testthat/test-tidyselect_fails_safely.R index d6e8c9745..72cf24b68 100644 --- a/tests/testthat/test-tidyselect_fails_safely.R +++ b/tests/testthat/test-tidyselect_fails_safely.R @@ -10,10 +10,10 @@ test_that("tidyselect errors signaled at report, not during development of valid expect_s3_class(a4 <- agent %>% col_vals_not_null(all_of(nonexistent_col)), "ptblank_agent") # Failure signaled via report - expect_message(expect_no_error(a1 %>% interrogate()), "ERROR") - expect_message(expect_no_error(a2 %>% interrogate()), "ERROR") - expect_message(expect_no_error(a3 %>% interrogate()), "ERROR") - expect_message(expect_no_error(a4 %>% interrogate()), "ERROR") + expect_false(a1 %>% interrogate() %>% all_passed()) + expect_false(a2 %>% interrogate() %>% all_passed()) + expect_false(a3 %>% interrogate() %>% all_passed()) + expect_false(a4 %>% interrogate() %>% all_passed()) # Stress testing expect_no_error(agent %>% col_vals_not_null(stop())) @@ -52,23 +52,25 @@ test_that("fail state correctly registered in the report for tidyselect errors", test_that("(tidy-)selecting 0 columns = skip the validation step at interrogation", { + eval_inactive <- function(x) !x$validation_set$eval_active + # Old behavior for vars()/NULL/ preserved: ## 1) No immediate error for zero columns selected expect_s3_class(a5 <- agent %>% col_vals_not_null(vars()), "ptblank_agent") expect_s3_class(a6 <- agent %>% col_vals_not_null(NULL), "ptblank_agent") expect_s3_class(a7 <- agent %>% col_vals_not_null(), "ptblank_agent") ## 2) # Treated as inactive in the report - expect_message(expect_no_error(a5 %>% interrogate()), "Skipping") - expect_message(expect_no_error(a6 %>% interrogate()), "Skipping") - expect_message(expect_no_error(a7 %>% interrogate()), "Skipping") + expect_true(a5 %>% interrogate() %>% eval_inactive()) + expect_true(a6 %>% interrogate() %>% eval_inactive()) + expect_true(a7 %>% interrogate() %>% eval_inactive()) # Same behavior of 0-column selection replicated in tidyselect patterns expect_length(small_table %>% dplyr::select(any_of("z")), 0) expect_length(small_table %>% dplyr::select(c()), 0) expect_s3_class(a8 <- agent %>% col_vals_not_null(any_of("z")), "ptblank_agent") expect_s3_class(a9 <- agent %>% col_vals_not_null(c()), "ptblank_agent") - expect_message(expect_no_error(a8 %>% interrogate()), "Skipping") - expect_message(expect_no_error(a9 %>% interrogate()), "Skipping") + expect_true(a8 %>% interrogate() %>% eval_inactive()) + expect_true(a9 %>% interrogate() %>% eval_inactive()) }) From 7e492bd3766fddbda77799e6cbf21db844b75555 Mon Sep 17 00:00:00 2001 From: June Choe Date: Fri, 20 Oct 2023 14:49:30 -0400 Subject: [PATCH 32/42] [chore] replace .env$ with all_of() --- R/table_transformers.R | 4 ++-- R/utils.R | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/R/table_transformers.R b/R/table_transformers.R index a38bb0fec..3e8f5684e 100644 --- a/R/table_transformers.R +++ b/R/table_transformers.R @@ -1058,9 +1058,9 @@ get_tt_param <- function( # Obtain the value from the `tbl` through a `select()`, `filter()`, `pull()` param_value <- tbl %>% - dplyr::select(.param., .env$column) %>% + dplyr::select(.param., tidyselect::all_of(column)) %>% dplyr::filter(.param. == .env$param) %>% - dplyr::pull(.env$column) + dplyr::pull(tidyselect::all_of(column)) } else if (tt_type == "tbl_dims") { diff --git a/R/utils.R b/R/utils.R index 3d85d8e6b..79b6fdfbf 100644 --- a/R/utils.R +++ b/R/utils.R @@ -345,7 +345,7 @@ resolve_segments <- function(x, seg_expr, preconditions) { col_seg_vals <- tbl %>% - dplyr::select(.env$column_name) %>% + dplyr::select(tidyselect::all_of(column_name)) %>% dplyr::distinct() %>% dplyr::pull() From bbacbc786d974a13340b345ca8c6b7f9dc0af96d Mon Sep 17 00:00:00 2001 From: June Choe Date: Sat, 21 Oct 2023 18:01:50 -0400 Subject: [PATCH 33/42] tidyselect columns in rows_complete --- R/rows_complete.R | 19 +++++-------------- 1 file changed, 5 insertions(+), 14 deletions(-) diff --git a/R/rows_complete.R b/R/rows_complete.R index c7fd201c4..ceb4c8327 100644 --- a/R/rows_complete.R +++ b/R/rows_complete.R @@ -278,22 +278,13 @@ rows_complete <- function( # Capture the `columns` expression columns <- rlang::enquo(columns) - - if (uses_tidyselect(expr_text = columns_expr)) { - - # Resolve the columns based on the expression - columns <- resolve_columns(x = x, var_expr = columns, preconditions = NULL) - - } else { - - # Resolve the columns based on the expression - if (!is.null(rlang::eval_tidy(columns)) && !is.null(columns)) { - columns <- resolve_columns(x = x, var_expr = columns, preconditions) - } else { - columns <- NULL - } + if (rlang::quo_is_null(columns)) { + columns <- rlang::quo(tidyselect::everything()) } + # Resolve the columns based on the expression + columns <- resolve_columns(x = x, var_expr = columns, preconditions = NULL) + # Resolve segments into list segments_list <- resolve_segments( From 56fb16f89f56e1e28a9f8996f81a966e7007dd1e Mon Sep 17 00:00:00 2001 From: June Choe Date: Sat, 21 Oct 2023 18:02:26 -0400 Subject: [PATCH 34/42] test 0-column selection behavior for rows_* validation functions --- tests/testthat/test-tidyselect_fails_safely.R | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/tests/testthat/test-tidyselect_fails_safely.R b/tests/testthat/test-tidyselect_fails_safely.R index 72cf24b68..4ad1c6ee3 100644 --- a/tests/testthat/test-tidyselect_fails_safely.R +++ b/tests/testthat/test-tidyselect_fails_safely.R @@ -74,6 +74,23 @@ test_that("(tidy-)selecting 0 columns = skip the validation step at interrogatio }) +test_that("tidyselecting 0 columns for rows_* functions = error at interrogation", { + + expect_no_error(a_rows_distinct <- agent %>% rows_distinct(starts_with("z")) %>% interrogate()) + expect_no_error(a_rows_complete <- agent %>% rows_distinct(starts_with("z")) %>% interrogate()) + expect_true(a_rows_distinct$validation_set$eval_error) + expect_true(a_rows_complete$validation_set$eval_error) + + # TODO: 0-column selection from tidyselect helpers *not* caught by `uses_tidyselect()` + # will still have same behavior but show different eval error message + # (errors from `select()` and not from `column_validity_has_columns()` as above) + expect_no_error(a_rows_distinct2 <- agent %>% rows_distinct(any_of("z")) %>% interrogate()) + expect_no_error(a_rows_complete2 <- agent %>% rows_distinct(any_of("z")) %>% interrogate()) + expect_true(a_rows_distinct2$validation_set$eval_error) + expect_true(a_rows_complete2$validation_set$eval_error) + +}) + test_that("tidyselect errors *are* immediate for assertion/expectation/test", { mismatch_msg <- "Can't subset columns that don't exist." From 4f07e535ca698c49c488ff131f570453ad4c779c Mon Sep 17 00:00:00 2001 From: June Choe Date: Sat, 21 Oct 2023 19:17:44 -0400 Subject: [PATCH 35/42] add june as aut --- DESCRIPTION | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/DESCRIPTION b/DESCRIPTION index abec80d86..8aac216e5 100644 --- a/DESCRIPTION +++ b/DESCRIPTION @@ -14,7 +14,9 @@ Authors@R: c( person("Richard", "Iannone", , "rich@posit.co", c("aut", "cre"), comment = c(ORCID = "0000-0003-3925-190X")), person("Mauricio", "Vargas", , "mavargas11@uc.cl", c("aut"), - comment = c(ORCID = "0000-0003-1017-7574")) + comment = c(ORCID = "0000-0003-1017-7574")), + person("June", "Choe", , "jchoe001@gmail.com", c("aut"), + comment = c(ORCID = "0000-0002-0701-921X")) ) License: MIT + file LICENSE URL: https://rstudio.github.io/pointblank/, https://github.com/rstudio/pointblank From 05853927aaa5e9bbbab9d416eb633ee042d6132a Mon Sep 17 00:00:00 2001 From: June Choe Date: Sat, 21 Oct 2023 20:12:01 -0400 Subject: [PATCH 36/42] use all_of --- R/rows_complete.R | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/R/rows_complete.R b/R/rows_complete.R index ceb4c8327..02632b58d 100644 --- a/R/rows_complete.R +++ b/R/rows_complete.R @@ -298,7 +298,7 @@ rows_complete <- function( secret_agent <- create_agent(x, label = "::QUIET::") %>% rows_complete( - columns = columns, + columns = tidyselect::all_of(columns), preconditions = preconditions, segments = segments, label = label, From f07e3960565c74318c4997baee1b4430761a6db0 Mon Sep 17 00:00:00 2001 From: June Choe Date: Sat, 21 Oct 2023 20:12:31 -0400 Subject: [PATCH 37/42] clean up tidyselect test file --- tests/testthat/test-tidyselect_integration.R | 56 ++++++++++++++++---- 1 file changed, 45 insertions(+), 11 deletions(-) diff --git a/tests/testthat/test-tidyselect_integration.R b/tests/testthat/test-tidyselect_integration.R index b141eda76..e8c90ae5f 100644 --- a/tests/testthat/test-tidyselect_integration.R +++ b/tests/testthat/test-tidyselect_integration.R @@ -1,6 +1,6 @@ test_that("Full range of tidyselect features available in column selection", { - tbl <- data.frame(x = 1:2, y = 1:2, yy = 1:2, nonunique = "A") + tbl <- data.frame(x = 1:2, y = 1:2, nonunique = "A") # Single symbol expect_success(expect_rows_distinct(tbl, x)) @@ -23,13 +23,13 @@ test_that("Full range of tidyselect features available in column selection", { # NEW: {tidyselect} integer indexing expect_success(expect_rows_distinct(tbl, 1)) - expect_success(expect_rows_distinct(tbl, c(1, 4))) - expect_failure(expect_rows_distinct(tbl, 4)) + expect_success(expect_rows_distinct(tbl, c(1, 3))) + expect_failure(expect_rows_distinct(tbl, 3)) # NEW: {tidyselect} negative indexing - expect_success(expect_rows_distinct(tbl, -(2:4))) expect_success(expect_rows_distinct(tbl, -(2:3))) - expect_failure(expect_rows_distinct(tbl, -(1:3))) + expect_success(expect_rows_distinct(tbl, -2)) + expect_failure(expect_rows_distinct(tbl, -(1:2))) # NEW: {tidyselect} `where()` predicate: expect_success(expect_rows_distinct(tbl, !tidyselect::where(is.character))) @@ -43,16 +43,50 @@ test_that("Full range of tidyselect features available in column selection", { expect_error(expect_rows_distinct(tbl, c(x, tidyselect::all_of(nonexist_col)))) expect_success(expect_rows_distinct(tbl, c(x, tidyselect::any_of(nonexist_col)))) - # DEPRECATION: Supplying a character vector variable still works, but signals deprecation: - options(lifecycle_verbosity = "warning") + # Supplying a character vector variable still works, but signals deprecation: + rlang::local_options(lifecycle_verbosity = "warning") expect_success(expect_warning( expect_rows_distinct(tbl, exist_col), "Using an external vector in selections was deprecated in tidyselect 1.1.0." )) - # For `rows_distinct()` specifically, NULL = "select everything" behavior implemented in tidyselect: - expect_success(expect_rows_distinct(tbl, )) - tbl_only_nonunique <- tbl[,"nonunique", drop = FALSE] - expect_failure(expect_rows_distinct(tbl_only_nonunique, )) +}) + + +test_that("'NULL = select everything' behavior in rows_*() validation functions", { + + # For `rows_*()` functions specifically, empty/NULL = "select everything" behavior: + expect_success(expect_rows_distinct(data.frame(x = 1, y = 2))) + expect_success(expect_rows_complete(data.frame(x = 1, y = 2))) + expect_failure(expect_rows_distinct(data.frame(x = c(1, 1)))) + expect_failure(expect_rows_complete(data.frame(x = c(1, NA)))) + expect_success(expect_rows_distinct(data.frame(x = 1, y = 2), columns = NULL)) + expect_success(expect_rows_complete(data.frame(x = 1, y = 2), columns = NULL)) + expect_failure(expect_rows_distinct(data.frame(x = c(1, 1)), columns = NULL)) + expect_failure(expect_rows_complete(data.frame(x = c(1, NA)), columns = NULL)) + + # Report shows all column names with empty `columns` argument + expect_equal({ + small_table %>% + create_agent() %>% + rows_distinct() %>% + rows_complete() %>% + interrogate() %>% + {.$validation_set$column} %>% + unlist() %>% + unique() + }, toString(colnames(small_table))) + # Report shows all column names with explicit NULL `columns` argument + expect_equal({ + small_table %>% + create_agent() %>% + rows_distinct(columns = NULL) %>% + rows_complete(columns = NULL) %>% + interrogate() %>% + {.$validation_set$column} %>% + unlist() %>% + unique() + }, toString(colnames(small_table))) + }) From 7b8c66c65ae04e3f5725181bb9d7c09dcfd05e84 Mon Sep 17 00:00:00 2001 From: June Choe Date: Sat, 21 Oct 2023 20:13:27 -0400 Subject: [PATCH 38/42] tests expect column info from row_complete() validation step --- tests/testthat/test-interrogate_with_agent.R | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/testthat/test-interrogate_with_agent.R b/tests/testthat/test-interrogate_with_agent.R index 4cc70dfed..ab34fa732 100644 --- a/tests/testthat/test-interrogate_with_agent.R +++ b/tests/testthat/test-interrogate_with_agent.R @@ -873,7 +873,7 @@ test_that("Interrogating for valid row values", { # Expect certain values in `validation$validation_set` expect_equivalent(validation$tbl_name, "small_table") expect_equivalent(validation$validation_set$assertion_type, "rows_complete") - expect_true(is.na(validation$validation_set$column %>% unlist())) + expect_equivalent(validation$validation_set$column %>% unlist(), "date_time, date, a, b, c, d, e, f") expect_true(is.null(validation$validation_set[["values"]][[1]])) expect_false(validation$validation_set$all_passed) expect_equivalent(validation$validation_set$n, 13) @@ -896,7 +896,7 @@ test_that("Interrogating for valid row values", { # Expect certain values in `validation$validation_set` expect_equivalent(validation$tbl_name, "small_table") expect_equivalent(validation$validation_set$assertion_type, "rows_complete") - expect_true(is.na(validation$validation_set$column %>% unlist())) + expect_equivalent(validation$validation_set$column %>% unlist(), "date_time, date, a, b, c, d, e, f") expect_true(is.null(validation$validation_set[["values"]][[1]])) expect_true(validation$validation_set$all_passed) expect_equivalent(validation$validation_set$n, 3) From 2255b41ea87ed4e138b0d4ad382466ababd2072a Mon Sep 17 00:00:00 2001 From: June Choe Date: Mon, 23 Oct 2023 14:24:35 -0400 Subject: [PATCH 39/42] tidyselect for col_exists(); resolves #433 --- R/col_exists.R | 17 +++++++---------- 1 file changed, 7 insertions(+), 10 deletions(-) diff --git a/R/col_exists.R b/R/col_exists.R index 058145ff6..ff1723630 100644 --- a/R/col_exists.R +++ b/R/col_exists.R @@ -234,17 +234,14 @@ col_exists <- function( rlang::as_label(rlang::quo(!!enquo(columns))) %>% gsub("^\"|\"$", "", .) - # Normalize the `columns` expression - if (inherits(columns, "quosures")) { - - columns <- - vapply( - columns, - FUN.VALUE = character(1), - USE.NAMES = FALSE, - FUN = function(x) as.character(rlang::get_expr(x)) - ) + # Capture the `columns` expression + columns <- rlang::enquo(columns) + if (rlang::quo_is_null(columns)) { + columns <- rlang::quo(tidyselect::everything()) } + + # Resolve the columns based on the expression + columns <- resolve_columns(x = x, var_expr = columns, preconditions = NULL) if (is_a_table_object(x)) { From be5725ecbe289ffa0f8dc082389184a26683e1e6 Mon Sep 17 00:00:00 2001 From: June Choe Date: Mon, 23 Oct 2023 14:24:56 -0400 Subject: [PATCH 40/42] test tidyselect for col_exists() --- tests/testthat/test-tidyselect_integration.R | 34 +++++++++++++++++++- 1 file changed, 33 insertions(+), 1 deletion(-) diff --git a/tests/testthat/test-tidyselect_integration.R b/tests/testthat/test-tidyselect_integration.R index e8c90ae5f..ac72c1db6 100644 --- a/tests/testthat/test-tidyselect_integration.R +++ b/tests/testthat/test-tidyselect_integration.R @@ -52,7 +52,6 @@ test_that("Full range of tidyselect features available in column selection", { }) - test_that("'NULL = select everything' behavior in rows_*() validation functions", { # For `rows_*()` functions specifically, empty/NULL = "select everything" behavior: @@ -90,3 +89,36 @@ test_that("'NULL = select everything' behavior in rows_*() validation functions" }, toString(colnames(small_table))) }) + +# tidyselect coverage for `col_exists()` +test_that("'NULL = select everything' behavior in rows_*() validation functions", { + + # Reprex from (#433) + df <- tibble::tibble( + id.x = 1:3, + id.y = 1:3, + stuff = 1:3 + ) + expect_success({ + df %>% + expect_col_exists( + columns = vars(ends_with(".x")) + ) + }) + expect_equal({ + df %>% + col_exists( + columns = vars(ends_with(".x")) + ) + }, df) + + # Multiple column selection produces multiple steps + expect_no_error({ + df_interrogated <- df %>% + create_agent() %>% + col_exists(starts_with("id")) %>% + interrogate() + }) + expect_equal(nrow(df_interrogated$validation_set), 2L) + +}) From 8ec5486071d3a14287836bb256cc06ad442de738 Mon Sep 17 00:00:00 2001 From: June Choe Date: Mon, 23 Oct 2023 14:58:00 -0400 Subject: [PATCH 41/42] more tests for vars() backwards compatibility --- tests/testthat/test-tidyselect_integration.R | 42 +++++++++++++++----- 1 file changed, 33 insertions(+), 9 deletions(-) diff --git a/tests/testthat/test-tidyselect_integration.R b/tests/testthat/test-tidyselect_integration.R index ac72c1db6..2e24f7ac4 100644 --- a/tests/testthat/test-tidyselect_integration.R +++ b/tests/testthat/test-tidyselect_integration.R @@ -1,16 +1,42 @@ -test_that("Full range of tidyselect features available in column selection", { +tbl <- data.frame(x = 1:2, y = 1:2, nonunique = "A") +exist_col <- "y" +nonunique_col <- "nonunique" +nonexist_col <- "z" + +test_that("Backwards compatibility with `vars()`", { + + # Bare symbol selects column(s) + expect_success(expect_rows_distinct(tbl, vars(x))) + expect_success(expect_rows_distinct(tbl, vars(x, nonunique))) + expect_failure(expect_rows_distinct(tbl, vars(nonunique))) + + # Bare character selects column(s) + expect_success(expect_rows_distinct(tbl, vars("x"))) + expect_success(expect_rows_distinct(tbl, vars("x", "nonunique"))) + expect_failure(expect_rows_distinct(tbl, vars("nonunique"))) + + # Bang-bang in-lines value + expect_success(expect_rows_distinct(tbl, vars(!!exist_col))) + expect_failure(expect_rows_distinct(tbl, vars(!!nonunique_col))) + + # `vars()` wrapping tidyselect expressions is redundant but continues to work + expect_success(expect_rows_distinct(tbl, vars(all_of("x")))) - tbl <- data.frame(x = 1:2, y = 1:2, nonunique = "A") + # `vars()` selection of 0-columns errors *only* in non-validation-planning contexts + expect_error(rows_distinct(tbl, vars("z"))) + expect_error(expect_rows_distinct(tbl, vars("z"))) + expect_error(test_rows_distinct(tbl, vars("z"))) + expect_no_error(tbl %>% create_agent() %>% rows_distinct(vars("z"))) + expect_no_error(tbl %>% create_agent() %>% rows_distinct(vars("z")) %>% interrogate()) + +}) + +test_that("Full range of tidyselect features available in column selection", { # Single symbol expect_success(expect_rows_distinct(tbl, x)) expect_failure(expect_rows_distinct(tbl, nonunique)) - # Backward-compatibility with `vars()` syntax - expect_success(expect_rows_distinct(tbl, vars(x))) - expect_success(expect_rows_distinct(tbl, vars(x, nonunique))) - expect_failure(expect_rows_distinct(tbl, vars(nonunique))) - # Preferred {tidyselect}-style `c()` syntax expect_success(expect_rows_distinct(tbl, c(x))) expect_success(expect_rows_distinct(tbl, c(x, nonunique))) @@ -37,9 +63,7 @@ test_that("Full range of tidyselect features available in column selection", { expect_failure(expect_rows_distinct(tbl, tidyselect::where(is.character))) # NEW: {tidyselect} functions in complex expressions - exist_col <- "y" expect_success(expect_rows_distinct(tbl, c(x, tidyselect::all_of(exist_col)))) - nonexist_col <- "z" expect_error(expect_rows_distinct(tbl, c(x, tidyselect::all_of(nonexist_col)))) expect_success(expect_rows_distinct(tbl, c(x, tidyselect::any_of(nonexist_col)))) From 3461c330b205554593b89187e9adae250cb54e8e Mon Sep 17 00:00:00 2001 From: June Choe Date: Mon, 23 Oct 2023 14:58:27 -0400 Subject: [PATCH 42/42] col_exists() only fails and never errors for missing column selection --- R/col_exists.R | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/R/col_exists.R b/R/col_exists.R index ff1723630..0fa0b3c60 100644 --- a/R/col_exists.R +++ b/R/col_exists.R @@ -241,7 +241,11 @@ col_exists <- function( } # Resolve the columns based on the expression - columns <- resolve_columns(x = x, var_expr = columns, preconditions = NULL) + ## Only for `col_exists()`: error gracefully if column not found + columns <- tryCatch( + expr = resolve_columns(x = x, var_expr = columns, preconditions = NULL), + error = function(cnd) cnd$i %||% NA_character_ + ) if (is_a_table_object(x)) {