-
Notifications
You must be signed in to change notification settings - Fork 112
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
Have steps return integers when appropriate #1039
Conversation
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.
Very much in support of the changes made here! These seem like a good move.🙂
I'm generally not very familiar with the internals of this package and how its dependencies handle its output, so I'm hesitant to approve merging without revdepchecking. Could we cloud_check
with these changes? I also ran checks with these changes on a few workflowsets and stacks setups to confirm that there aren't any unanticipated side effects here, especially with how dependencies handle the classes of step_dummy
and step_integer
output, and saw no breakages!
Co-authored-by: Simon P. Couch <simonpatrickcouch@gmail.com>
Co-authored-by: Simon P. Couch <simonpatrickcouch@gmail.com>
I just finished running a full revdepcheck on this PR and it ran clean. I'm not surprised since recipes currently doesn't really have a way to detect/use integers. #993 on the other hand might break some stuff. |
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.
Okay, awesome. Thanks for your patience with my hesitance—looks great!☃️
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. |
To close #766.
To be closed before #993 to in according to: #993 (comment).
This PR looks at the following steps:
These steps have their
bake.step_*()
modified to return integers when it couldstep_date()
step_dummy()
step_dummy_extract()
step_holiday()
step_ordinalscore()
step_regex()
These steps have had their argument defaults changed to produce integers by default
step_intercept()
step_integer()
These steps already returned integers when needed, got tests to verify
step_indicate_na()
step_count()
step_time()
Nothing changed, included for completeness
step_dummy_multi_choice()