-
Notifications
You must be signed in to change notification settings - Fork 418
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
Implement nest(.by = )
and revive .key
#1461
Conversation
@@ -106,39 +156,52 @@ nest <- function(.data, ..., .names_sep = NULL, .key = deprecated()) { | |||
i = "Did you want `{(.key)} = {cols_fixed_label}`?" | |||
)) | |||
|
|||
return(nest(.data, !!!cols)) | |||
return(nest(.data, !!!cols, .by = {{ .by }})) |
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.
Supports df %>% nest(y, .by = x)
where we would get the warning about forgetting data = y
but then it would continue as expected
out <- vec_cbind(out, inner, .name_repair = "check_unique", .error_call = error_call) | ||
out <- reconstruct_tibble(.data, out) |
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.
vec_cbind()
with a grouped data frame will drop the <grouped_df>
class and return a bare tibble. So we use reconstruct_tibble()
after that to restore the grouped class if possible. There are tests for this.
Both pack()
and chop()
also do reconstruct_tibble()
on the way out so we don't need it after them
NextMethod() | ||
nest.tbl_df(.data, ..., .key = .key, .names_sep = .names_sep) |
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 was a bit unfortunate. Adding a tidy-select .by
argument means that you can't call NextMethod()
anymore, you have to recall the generic (or the method directly, as is done here).
This did not break any S3 methods out there as far as I can tell.
test_that("catches when `...` overwrites an existing column", { | ||
df <- tibble(x = 1, y = 2) | ||
|
||
# Hardcoded as an error. | ||
# Name repair would likely break internal usage of `chop()`. | ||
expect_snapshot(error = TRUE, { | ||
nest(df, x = y) | ||
}) | ||
}) |
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 was previously not being caught, and was giving an even more obscure error from chop()
c673b9a
to
ab2673d
Compare
if (identical(maybe_missing(key), deprecated())) { | ||
# Temporary support for S3 method authors that set `.key = deprecated()`. | ||
# Remove this entire helper all methods have been updated. | ||
key <- NULL | ||
} |
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.
My "hack" to catch .key = deprecated()
from other S3 methods
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.
Maybe we've finally gotten the nesting interface right!
@@ -106,39 +151,52 @@ nest <- function(.data, ..., .names_sep = NULL, .key = deprecated()) { | |||
i = "Did you want `{(.key)} = {cols_fixed_label}`?" |
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.
Should we switch this to lifecycle_warn(always = TRUE)
? Or maybe even make it defunct? It was deprecated in tidyr 1.0.0 (August 2019), so it's been >3 years with a warning on every use.
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.
Hmm I'll fiddle with that in a follow up PR
ce26585
to
e0a29fa
Compare
Closes #1458
Also has these two cool new features:
This also revives the
.key
argument, with a default ofNULL
. This is a bit tricky because it used to bedeprecated()
, and if S3 method authors updated their methods to also say.key = deprecated()
as the default, then their code may break. I've handled this by silently convertingdeprecated()
toNULL
..key
is used whenever...
isn't specified. This happens in 3 ways:nest(df)
where we nest everythingnest(df, .by = x)
where we specify columns to nest bydf %>% group_by(x) %>% nest()
where columns to nest by are implied by the groupsIf both
...
and.key
are specified, a warning is thrown saying.key
is ignored.The only revdep that breaks because of these changes is panelr, because they have sticky
[
methods and we now usedf[cols]
innest()
. I can send them a PR after we merge this.