Add error messaging when a step argument that cannot be varying is set to varying() (#135)

Author: DavisVaughan

NEWS.md

@@ -2,6 +2,14 @@
 
 ## Bug fixes
 
+* For the recipes step method of `varying_args()`, there is now error checking
+to catch if a user tries to specify an argument that _cannot_ be varying as 
+varying (for example, the `id`) (#132).
+
+* `find_varying()`, the internal function for detecting varying arguments, 
+now returns correct results when a size 0 argument is provided. It can also now
+detect varying arguments nested deeply into a call (#131, #134).
+
 * For multinomial regression, the `.pred_` prefix is now only added to prediction
 column names once (#107).
 

R/varying.R

@@ -101,20 +101,32 @@ varying_args.recipe <- function(x, id = NULL, ...) {
 #' @rdname varying_args
 varying_args.step <- function(x, id = NULL, ...) {
   cl <- match.call()
-  if (!is.null(id) && !is.character(id))
+
+  if (!is.null(id) && !is.character(id)) {
     stop ("`id` should be a single character string.", call. = FALSE)
+  }
+
   id <- id[1]
 
-  if (is.null(id))
+  if (is.null(id)) {
     id <- deparse(cl$x)
+  }
+
+  # Grab the step class before the subset, as that removes the class
+  step_type <- class(x)[1]
 
-  exclude <-
-    c("terms", "role", "trained", "skip", "na.rm", "impute_with", "seed",
-      "prefix", "naming", "denom", "outcome", "id")
-  x <- x[!(names(x) %in% exclude)]
+  # Remove NULL argument steps. These are reserved
+  # for deprecated args or those set at prep() time.
   x <- x[!map_lgl(x, is.null)]
+
   res <- map_lgl(x, find_varying)
 
+  # ensure the user didn't specify a non-varying argument as varying()
+  validate_only_allowed_step_args(res, step_type)
+
+  # remove the non-varying arguments as they are not important
+  res <- res[!(names(x) %in% non_varying_step_arguments)]
+
   tibble(
     name = names(res),
     varying = unname(res),
@@ -123,6 +135,37 @@ varying_args.step <- function(x, id = NULL, ...) {
   )
 }
 
+validate_only_allowed_step_args <- function(x, step_type) {
+
+  check_allowed_arg <- function(x, nm) {
+
+    # not varying
+    if (rlang::is_false(x)) {
+      return(invisible(x))
+    }
+
+    # not a non-varying step arg name
+    bad_nm <- nm %in% non_varying_step_arguments
+    if (!bad_nm) {
+      return(invisible(x))
+    }
+
+    rlang::abort(glue::glue(
+      "The following argument for a recipe step of type ",
+      "'{step_type}' is not allowed to vary: '{nm}'."
+    ))
+  }
+
+  purrr::iwalk(x, check_allowed_arg)
+  invisible(x)
+}
+
+non_varying_step_arguments <- c(
+  "terms", "role", "trained", "skip",
+  "na.rm", "impute_with", "seed",
+  "prefix", "naming", "denom", "outcome", "id"
+)
+
 # helpers ----------------------------------------------------------------------
 
 is_varying <- function(x) {

tests/testthat/test_varying.R

@@ -147,3 +147,14 @@ test_that("varying() deeply nested in calls can be located - #134", {
     TRUE
   )
 })
+
+test_that("recipe steps with non-varying args error if specified as varying()", {
+
+  rec_bad_varying <- rec_1
+  rec_bad_varying$steps[[1]]$skip <- varying()
+
+  expect_error(
+    varying_args(rec_bad_varying),
+    "The following argument for a recipe step of type 'step_center' is not allowed to vary: 'skip'."
+  )
+})