-
Notifications
You must be signed in to change notification settings - Fork 1
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
Allow workflows with formulas #59
base: main
Are you sure you want to change the base?
Conversation
@@ -0,0 +1,83 @@ | |||
# --- | |||
# repo: tidymodels/workflows |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This "standalone" file is in the next version of workflows and is designed to be added to other packages. It is tested there and the nocov
directives prevent it from decreasing code coverage here.
@@ -1,19 +1,17 @@ | |||
# read in data to use in tests |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I moved these to a "helper.R" file. Since it starts with "helper" it gets executed prior to each test file.
) | ||
|
||
# predictors missing from new_data | ||
expect_error( | ||
expect_snapshot( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I moved most of the expect_error()
code to expect_snapshot()
since it tends to be more robust to unrelated changes to the package.
) | ||
|
||
# tests | ||
expect_s3_class(x, c("tbl_df", "tbl", "data.frame")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could reduce a lot of these by making a "ptype" list which is just a zero-row slice of the data (e.g. x[0,]
). That contains the classes of the overall object as well as the columns.
@@ -235,7 +237,7 @@ ames_boot_conf_int %>% | |||
geom_point(aes(y = Sale_Price), | |||
alpha = 0.25) + | |||
scale_x_log10(labels = scales::comma_format()) + | |||
scale_y_log10(labels = scales::label_number(scale_cut = scales::cut_short_scale())) + |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These were causing an error. I, for the life of me, could not find a way to resolve it. I might be able to ask Thomas about it.
Closes #56
The main API change is that a fitted workflow would now be required. That made the changes a little more invasive than I hoped they would be since it affects documentation and testing.
I could make the testing code a little more simple with some changes that would avoid a bunch of
expect_type()
andexpect_s3_class()
calls. Now sure how much more you would like me to make more changes.On the bright side:
Created on 2024-01-29 with reprex v2.0.2