Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

A more informative error when the user accidentally spreads levels in col_factor(...) #1535

Open
yjunechoe opened this issue Mar 27, 2024 · 0 comments

Comments

@yjunechoe
Copy link

yjunechoe commented Mar 27, 2024

Issue

In col_factor(), levels of the factor must be specified in the first argument levels, supplied as a vector:

x <- c("M", "F", NA)

collector <- col_factor(c("M", "F"))

parse_vector(x, collector)
#> [1] M    F    <NA>
#> Levels: M F

In practice, users may accidentally spread out the levels across arguments of col_factor(). In current implementation, the collector is still successfully constructed, and the error is only caught at the point of parsing, with a rather cryptic message.

collector <- col_factor("M", "F")

parse_vector(x, collector)
#> Error: Expected single logical value

In so far as the second argument of col_factor() (ordered) expects a scalar logical, I think it'd be helpful for col_factor() to check for this early at its creation. Additionally, it would be helpful for col_factor() to inform the likely source of the error if ordered receives a scalar character instead (the user mistakenly treated levels as ...).

Proposed solution

A behavior like this would be helpful for debugging:

col_factor("M", "F")
#> Error in `col_factor()`:
#> ! `ordered` must be a logical, not a string.
#> ℹ Did you intend to pass a vector of levels to `levels`?

And here's the toy implementation for the above (inserts an if block to check ordered):

col_factor <- function(levels = NULL, ordered = FALSE, include_na = FALSE) {
  if (!(is.null(levels) || is.character(levels))) {
    stop(sprintf("`levels` must be `NULL` or a character vector:\n- `levels` is a '%s'", class(levels)), call. = FALSE)
  }
  
  if (!rlang::is_scalar_logical(ordered)) {
    cli::cli_abort(c(
      "{.arg ordered} must be a logical, not {.obj_type_friendly {ordered}}.",
      if (rlang::is_scalar_character(ordered)) {
        c("i" = "Did you intend to pass a vector of levels to {.arg levels}?")
      }
    ))
  }
  
  collector("factor", levels = levels, ordered = ordered, include_na = include_na)
}

Happy to PR this if this is within scope!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant