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

Add step_relocate #162

Merged
merged 14 commits into from Jan 28, 2021
Merged

Add step_relocate #162

merged 14 commits into from Jan 28, 2021

Conversation

smingerson
Copy link
Contributor

@smingerson smingerson commented Aug 8, 2020

Combines code from step_select() and dplyr::relocate() to create step_relocate(). I added one basic test, unsure if more are needed. Also thought that maybe @import dplyr relocate needs to happen conditioned on dplyr version, but not quite sure how to handle that.

Edit: The failing tests are due to a change in tibble 3.0: glimpse() uses "Rows" and "Columns" instead of "Variables" and "Observations", because we're not sure if the data is tidy here (#614).

Copy link
Member

@hadley hadley left a comment

Choose a reason for hiding this comment

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

Thanks for working on this!

R/step-subset.R Outdated Show resolved Hide resolved
R/step-subset.R Outdated Show resolved Hide resolved
R/step-subset.R Outdated Show resolved Hide resolved
@@ -38,6 +38,11 @@ test_that("simple calls generate expected translations", {
expr(DT[, .(a = x, y)])
)

expect_equal(
Copy link
Member

Choose a reason for hiding this comment

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

I think relocate() is complicated enough that it should have it's own test case — it needs to have an example of exactly one of .before and .after, neither of them, and both of them. I'd also look at https://github.com/tidyverse/dplyr/blob/master/tests/testthat/test-relocate.R to see if there are other tricky cases we should also test for here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've added tests based off dplyr, as suggested. I also added one to ensure relocation does not change the grouping variables.

@hadley hadley merged commit c04b4a5 into tidyverse:master Jan 28, 2021
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

Successfully merging this pull request may close these issues.

None yet

2 participants