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

switch check_type() to use cli package #1235

Merged
merged 6 commits into from Oct 13, 2023
Merged

switch check_type() to use cli package #1235

merged 6 commits into from Oct 13, 2023

Conversation

EmilHvitfeldt
Copy link
Member

to close #1217.

This PR switches check_type() to use {cli} in its errors. Additionally and more importantly, it lists the offending variables and their types 🎉

library(recipes)
library(modeldata)

recipe(~., data = iris) |>
  step_date(all_predictors()) |>
  prep()
#> Error in `step_date()`:
#> Caused by error in `prep()`:
#> ✖ All columns selected for the step should be date, or datetime.
#> • 4 double variables found: `Sepal.Length`, `Sepal.Width`, …
#> • 1 factor variable found: `Species`

recipe(~., data = ames) |>
  step_date(all_predictors()) |>
  prep()
#> Error in `step_date()`:
#> Caused by error in `prep()`:
#> ✖ All columns selected for the step should be date, or datetime.
#> • 40 factor variables found: `MS_SubClass`, `MS_Zoning`, `Street`, …
#> • 12 double variables found: `Lot_Frontage`, `Mas_Vnr_Area`, …
#> • 22 integer variables found: `Lot_Area`, `Year_Built`, …

R/misc.R Outdated
classes <- map_chr(info$type, function(x) x[1])
counts <- vctrs::vec_split(info$variable, classes)
counts$count <- lengths(counts$val)
counts$text_len <- cli::console_width() - 19 - (counts$count > 1) -
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

-19 - my count of the length of "• variable found: "
(counts$count > 1) - whether variable has an s or not
nchar(counts$key) - length of the variable type

variable_collapse <- function(x, width) {
x <- paste0("{.var ", x, "}")
if (length(x) == 1) {
res <- cli::ansi_strtrim(x, width = width)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in the unlikely event that the single variable name is too large, i used cli::ansi_strtrim to truncate the variable by itself

Copy link
Contributor

@simonpcouch simonpcouch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a big improvement! Feel free to re-request review when you feel this is in a good spot. :)

R/misc.R Outdated Show resolved Hide resolved
tests/testthat/_snaps/factor2string.md Outdated Show resolved Hide resolved
R/misc.R Show resolved Hide resolved
Copy link
Contributor

@simonpcouch simonpcouch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, looks good! Looks like there's one issue with duplicated column names; once that's resolved, thumbs up from me.

R/misc.R Show resolved Hide resolved
@@ -23,7 +25,8 @@
Condition
Error in `step_ratio()`:
Caused by error in `prep()`:
! All columns selected for the step should be double, or integer.
x All columns selected for the step should be double or integer.
* 8 factor variables found: `x5`, `x5`, `x5`, `x5`, ...
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's happening here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

solved with a call to unique() 0a36bb7 (#1235)

@EmilHvitfeldt EmilHvitfeldt merged commit 26a7ed8 into main Oct 13, 2023
9 checks passed
@EmilHvitfeldt EmilHvitfeldt deleted the cli-check_type branch October 13, 2023 01:25
@github-actions
Copy link

This pull request has been automatically locked. If you believe you have found a related problem, please file a new issue (with a reprex https://reprex.tidyverse.org) and link to this issue.

@github-actions github-actions bot locked and limited conversation to collaborators Oct 28, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

check_type() should list offending variables
2 participants