bug(?): validators must return NULL

[JosiahParry]

I have observed that validators must return NULL otherwise the validators error out. I cannot reason as to why this is. Is this intended behavior? Should the validators always return self?

params <- S7::new_class(
  "form_params",
  properties = list(params = S7::class_list),
  validator = function(self) {
    lapply(
      self@params,
      rlang:::check_string,
      allow_null = TRUE,
      allow_na = FALSE,
      call = rlang::caller_call()
    )
    # idk why this has to return NULL
    return(NULL)
  }
)

params(list(x = "a"))
#> <form_params>
#>  @ params:List of 1
#>  .. $ x: chr "a"

params <- S7::new_class(
  "form_params",
  properties = list(params = S7::class_list),
  validator = function(self) {
    lapply(
      self@params,
      rlang:::check_string,
      allow_null = TRUE,
      allow_na = FALSE,
      call = rlang::caller_call()
    )
  }
)

params(list(x = "a"))
#> Error: <form_params> object is invalid:
#> - NULL

[hadley]

That's because they can return a string to more easily create an error.

[t-kalinowski]

The pattern that's easiest is to use c() to collapse NULLs. See S7:::validate_factor() for an example. This pattern makes it easy to generate an informative error message of everything that's wrong on a single validate call.

validate_factor <- function(self) {
  c(
    if (typeof(self) != "integer")
      "Underlying data must be an <integer>",
    if (!is.character(attr(self, "levels", TRUE)))
      "attr(, 'levels') must be a <character>",
    { rng <- range(unclass(self), 0L); NULL },
    if (rng[1] < 0L)
      "Underlying data must be all positive",
    if (rng[2] > length(attr(self, "levels", TRUE)))
      "Not enough 'levels' for underlying data"
  )
}

(Side-note: technically, the check on the validator returned value is for length() and not is.null(), returning a length-0 atomic character is fine too).

Also, IMO, the pattern of evaluating an expression "in-line" in the c() call, like rng <- range() in validate_factor(), is near the border of good practice/taste. For more extensive validator checks, this pattern might be better:

my_validator <- function(self) {
  .things_wrong <- character()
  invalid <- function(...) .things_wrong <<- c(.things_wrong, paste0(...))
  
  if(check1(self))
    invalid("thing1")
  if(check2(self))
    invalid("thing2")
  if(check3(self))
    invalid("thing3")
  
  .things_wrong
}

[JosiahParry]

For posterity this comes from:

"A validator is a function that takes the object (called self) and returns NULL if its valid or returns a character vector listing the problems." Classes and objects

Perhaps there can be checks or a more informative error message when the validator returns an unexpected object. I know I will forget about this in a month or so and find this issue.

The current behavior works to the letter of the law but it does feel like the validator has to be coaxed

[t-kalinowski]

I agree, we should emit a better error message if the validator returns something other than NULL or a character vector.