Skip to content

Commit 9c17094

Browse files
committed
Remove unnecessary/misleading select.epi_df implementation
`select.epi_df` looked weird: - `reclass(selected, attr(selected, "metadata"))` would only have made sense if grouped_df had a `select` that dropped our class but not our metadata, implemented in a way that called our own impls for `dplyr_extending` that would make that metadata actually be correct (in the case of renaming). - (`dplyr_reconstruct(selected, selected)` might have made sense, if `reclass` acted on something that shouldn't actually become an `epi_df`, decaying back to a non-`epi_df` only if needed.) So it seemed like it relied on `dplyr_extending` being almost, but not quite, sufficient. But it looks like `grouped_df` doesn't even have a `select` impl, so it's not interfering anyway! So it looks like we can just get rid of our own impl.
1 parent cfeda76 commit 9c17094

File tree

3 files changed

+20
-10
lines changed

3 files changed

+20
-10
lines changed

NAMESPACE

-1
Original file line numberDiff line numberDiff line change
@@ -38,7 +38,6 @@ S3method(next_after,integer)
3838
S3method(print,epi_archive)
3939
S3method(print,epi_df)
4040
S3method(print,grouped_epi_archive)
41-
S3method(select,epi_df)
4241
S3method(summary,epi_df)
4342
S3method(ungroup,epi_df)
4443
S3method(ungroup,grouped_epi_archive)

R/group_by_epi_df_methods.R

-8
Original file line numberDiff line numberDiff line change
@@ -3,11 +3,3 @@
33
# `epi_df`s. It would be nice if there were a way to replace these with a
44
# generic core that automatically fixed all misbehaving methods; see
55
# brainstorming within Issue #223.
6-
7-
#' @importFrom dplyr select
8-
#' @export
9-
select.epi_df <- function(.data, ...) {
10-
selected <- NextMethod(.data)
11-
might_decay <- reclass(selected, attr(selected, "metadata"))
12-
return(dplyr_reconstruct(might_decay, might_decay))
13-
}

tests/testthat/test-methods-epi_df.R

+20-1
Original file line numberDiff line numberDiff line change
@@ -173,11 +173,30 @@ test_that("Renaming columns while grouped gives appropriate colnames and metadat
173173
expect_identical(attr(renamed_gedf1, "metadata")$other_keys, c("age_group"))
174174
# renaming using select
175175
renamed_gedf2 <- gedf %>%
176-
as_epi_df(additional_metadata = list(other_keys = "age")) %>%
177176
select(geo_value, time_value, age_group = age, value)
178177
expect_identical(renamed_gedf1, renamed_gedf2)
179178
})
180179

180+
test_that("Additional `select` on `epi_df` tests", {
181+
edf <- tibble::tibble(geo_value = "ak", time_value = as.Date("2020-01-01"), age = 1, value = 1) %>%
182+
as_epi_df(additional_metadata = list(other_keys = "age"))
183+
184+
# Dropping a non-geo_value epikey column doesn't decay, though maybe it
185+
# should, since you'd expect that to possibly result in multiple rows per
186+
# epikey (though not in this toy case), and while we don't require that, we
187+
# sort of expect it:
188+
edf_not_decayed <- edf %>%
189+
select(geo_value, time_value, value)
190+
expect_class(edf_not_decayed, "epi_df")
191+
expect_identical(attr(edf_not_decayed, "metadata")$other_keys, character(0L))
192+
193+
# Dropping geo_value does decay:
194+
edf_decayed <- edf %>%
195+
select(age, time_value, value)
196+
expect_false(inherits(edf_decayed, "epi_df"))
197+
expect_identical(attr(edf_decayed, "metadata"), NULL)
198+
})
199+
181200
test_that("complete.epi_df works", {
182201
start_date <- as.Date("2020-01-01")
183202
daily_edf <- tibble::tribble(

0 commit comments

Comments
 (0)