Skip to content

Commit e61e11a

Browse files
dshemetovdshemetov
committed
refactor: changes from review
* remove is_epi_archive and delete in epix_slide * simplify group_by_drop_default * prune library calls in tests * remove here and waldo from Suggests * pull most validation work from new_epi_archive into validate_epi_archive * call validate_epi_archive in as_epi_archive
1 parent 5ea168e commit e61e11a

16 files changed

+102
-156
lines changed

‎DESCRIPTION‎

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -51,12 +51,10 @@ Suggests:
5151
covidcast,
5252
devtools,
5353
epidatr,
54-
here,
5554
knitr,
5655
outbreaks,
5756
rmarkdown,
5857
testthat (>= 3.1.5),
59-
waldo (>= 0.3.1),
6058
withr
6159
VignetteBuilder:
6260
knitr

‎NAMESPACE‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -64,7 +64,6 @@ export(filter)
6464
export(group_by)
6565
export(group_modify)
6666
export(growth_rate)
67-
export(is_epi_archive)
6867
export(is_epi_df)
6968
export(is_grouped_epi_archive)
7069
export(key_colnames)
@@ -78,6 +77,7 @@ export(rename)
7877
export(slice)
7978
export(ungroup)
8079
export(unnest)
80+
export(validate_epi_archive)
8181
importFrom(checkmate,anyInfinite)
8282
importFrom(checkmate,anyMissing)
8383
importFrom(checkmate,assert)
@@ -94,6 +94,7 @@ importFrom(checkmate,checkInt)
9494
importFrom(checkmate,check_atomic)
9595
importFrom(checkmate,check_data_frame)
9696
importFrom(checkmate,check_names)
97+
importFrom(checkmate,expect_class)
9798
importFrom(checkmate,test_set_equal)
9899
importFrom(checkmate,test_subset)
99100
importFrom(checkmate,vname)

‎R/archive.R‎

Lines changed: 79 additions & 82 deletions
Original file line numberDiff line numberDiff line change
@@ -307,59 +307,8 @@ new_epi_archive <- function(
307307
other_keys = NULL,
308308
additional_metadata = NULL,
309309
compactify = NULL,
310-
clobberable_versions_start = NA,
310+
clobberable_versions_start = NULL,
311311
versions_end = NULL) {
312-
assert_data_frame(x)
313-
if (!test_subset(c("geo_value", "time_value", "version"), names(x))) {
314-
cli_abort(
315-
"Columns `geo_value`, `time_value`, and `version` must be present in `x`."
316-
)
317-
}
318-
if (anyMissing(x$version)) {
319-
cli_abort("Column `version` must not contain missing values.")
320-
}
321-
322-
geo_type <- geo_type %||% guess_geo_type(x$geo_value)
323-
time_type <- time_type %||% guess_time_type(x$time_value)
324-
other_keys <- other_keys %||% character(0L)
325-
additional_metadata <- additional_metadata %||% list()
326-
327-
# Finish off with small checks on keys variables and metadata
328-
if (!test_subset(other_keys, names(x))) {
329-
cli_abort("`other_keys` must be contained in the column names of `x`.")
330-
}
331-
if (any(c("geo_value", "time_value", "version") %in% other_keys)) {
332-
cli_abort("`other_keys` cannot contain \"geo_value\", \"time_value\", or \"version\".")
333-
}
334-
if (any(names(additional_metadata) %in% c("geo_type", "time_type"))) {
335-
cli_warn("`additional_metadata` names overlap with existing metadata fields \"geo_type\", \"time_type\".")
336-
}
337-
338-
# Conduct checks and apply defaults for `compactify`
339-
assert_logical(compactify, len = 1, any.missing = FALSE, null.ok = TRUE)
340-
341-
# Apply defaults and conduct checks for
342-
# `clobberable_versions_start`, `versions_end`:
343-
versions_end <- versions_end %||% max_version_with_row_in(x)
344-
validate_version_bound(clobberable_versions_start, x, na_ok = TRUE)
345-
validate_version_bound(versions_end, x, na_ok = FALSE)
346-
if (nrow(x) > 0L && versions_end < max(x[["version"]])) {
347-
cli_abort(
348-
"`versions_end` was {versions_end}, but `x` contained
349-
updates for a later version or versions, up through {max(x$version)}",
350-
class = "epiprocess__versions_end_earlier_than_updates"
351-
)
352-
}
353-
if (!is.na(clobberable_versions_start) && clobberable_versions_start > versions_end) {
354-
cli_abort(
355-
"`versions_end` was {versions_end}, but a `clobberable_versions_start`
356-
of {clobberable_versions_start} indicated that there were later observed versions",
357-
class = "epiprocess__versions_end_earlier_than_clobberable_versions_start"
358-
)
359-
}
360-
361-
# --- End of validation and replacing missing args with defaults ---
362-
363312
# Create the data table; if x was an un-keyed data.table itself,
364313
# then the call to as.data.table() will fail to set keys, so we
365314
# need to check this, then do it manually if needed
@@ -441,18 +390,91 @@ new_epi_archive <- function(
441390
)
442391
}
443392

393+
#' `validate_epi_archive` ensures correctness of arguments fed to `as_epi_archive`.
394+
#'
395+
#' @rdname epi_archive
396+
#'
397+
#' @export
398+
validate_epi_archive <- function(
399+
x,
400+
geo_type = NULL,
401+
time_type = NULL,
402+
other_keys = NULL,
403+
additional_metadata = NULL,
404+
compactify = NULL,
405+
clobberable_versions_start = NULL,
406+
versions_end = NULL) {
407+
# Finish off with small checks on keys variables and metadata
408+
if (!test_subset(other_keys, names(x))) {
409+
cli_abort("`other_keys` must be contained in the column names of `x`.")
410+
}
411+
if (any(c("geo_value", "time_value", "version") %in% other_keys)) {
412+
cli_abort("`other_keys` cannot contain \"geo_value\", \"time_value\", or \"version\".")
413+
}
414+
if (any(names(additional_metadata) %in% c("geo_type", "time_type"))) {
415+
cli_warn("`additional_metadata` names overlap with existing metadata fields \"geo_type\", \"time_type\".")
416+
}
417+
418+
# Conduct checks and apply defaults for `compactify`
419+
assert_logical(compactify, len = 1, any.missing = FALSE, null.ok = TRUE)
420+
421+
# Apply defaults and conduct checks for
422+
# `clobberable_versions_start`, `versions_end`:
423+
validate_version_bound(clobberable_versions_start, x, na_ok = TRUE)
424+
validate_version_bound(versions_end, x, na_ok = FALSE)
425+
if (nrow(x) > 0L && versions_end < max(x[["version"]])) {
426+
cli_abort(
427+
"`versions_end` was {versions_end}, but `x` contained
428+
updates for a later version or versions, up through {max(x$version)}",
429+
class = "epiprocess__versions_end_earlier_than_updates"
430+
)
431+
}
432+
if (!is.na(clobberable_versions_start) && clobberable_versions_start > versions_end) {
433+
cli_abort(
434+
"`versions_end` was {versions_end}, but a `clobberable_versions_start`
435+
of {clobberable_versions_start} indicated that there were later observed versions",
436+
class = "epiprocess__versions_end_earlier_than_clobberable_versions_start"
437+
)
438+
}
439+
}
440+
444441

445442
#' `as_epi_archive` converts a data frame, data table, or tibble into an
446443
#' `epi_archive` object.
447444
#'
448445
#' @rdname epi_archive
449446
#'
450447
#' @export
451-
as_epi_archive <- function(x, geo_type = NULL, time_type = NULL, other_keys = NULL,
452-
additional_metadata = list(),
453-
compactify = NULL,
454-
clobberable_versions_start = NA,
455-
versions_end = max_version_with_row_in(x)) {
448+
as_epi_archive <- function(
449+
x,
450+
geo_type = NULL,
451+
time_type = NULL,
452+
other_keys = NULL,
453+
additional_metadata = NULL,
454+
compactify = NULL,
455+
clobberable_versions_start = NULL,
456+
versions_end = NULL) {
457+
assert_data_frame(x)
458+
if (!test_subset(c("geo_value", "time_value", "version"), names(x))) {
459+
cli_abort(
460+
"Columns `geo_value`, `time_value`, and `version` must be present in `x`."
461+
)
462+
}
463+
if (anyMissing(x$version)) {
464+
cli_abort("Column `version` must not contain missing values.")
465+
}
466+
467+
geo_type <- geo_type %||% guess_geo_type(x$geo_value)
468+
time_type <- time_type %||% guess_time_type(x$time_value)
469+
other_keys <- other_keys %||% character(0L)
470+
additional_metadata <- additional_metadata %||% list()
471+
clobberable_versions_start <- clobberable_versions_start %||% NA
472+
versions_end <- versions_end %||% max_version_with_row_in(x)
473+
474+
validate_epi_archive(
475+
x, geo_type, time_type, other_keys, additional_metadata,
476+
compactify, clobberable_versions_start, versions_end
477+
)
456478
new_epi_archive(
457479
x, geo_type, time_type, other_keys, additional_metadata,
458480
compactify, clobberable_versions_start, versions_end
@@ -652,31 +674,6 @@ group_by.epi_archive <- function(.data, ..., .add = FALSE, .drop = dplyr::group_
652674
}
653675

654676

655-
#' Test for `epi_archive` format
656-
#'
657-
#' @param x An object.
658-
#' @param grouped_okay Optional; Boolean; should a `grouped_epi_archive` also
659-
#' count? Default is `FALSE`.
660-
#' @return `TRUE` if the object inherits from `epi_archive`.
661-
#'
662-
#' @export
663-
#' @examples
664-
#' is_epi_archive(jhu_csse_daily_subset) # FALSE (this is an epi_df, not epi_archive)
665-
#' is_epi_archive(archive_cases_dv_subset) # TRUE
666-
#'
667-
#' # By default, grouped_epi_archives don't count as epi_archives, as they may
668-
#' # support a different set of operations from regular `epi_archives`. This
669-
#' # behavior can be controlled by `grouped_okay`.
670-
#' grouped_archive <- archive_cases_dv_subset %>% group_by(geo_value)
671-
#' is_epi_archive(grouped_archive) # FALSE
672-
#' is_epi_archive(grouped_archive, grouped_okay = TRUE) # TRUE
673-
#'
674-
#' @seealso [`is_grouped_epi_archive`]
675-
is_epi_archive <- function(x, grouped_okay = FALSE) {
676-
inherits(x, "epi_archive") || grouped_okay && inherits(x, "grouped_epi_archive")
677-
}
678-
679-
680677
#' Clone an `epi_archive` object.
681678
#'
682679
#' @param x An `epi_archive` object.

‎R/epiprocess.R‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,7 @@
77
#' @importFrom checkmate assert assert_scalar assert_data_frame anyMissing
88
#' assert_logical assert_list assert_character assert_class
99
#' assert_int assert_numeric check_data_frame vname check_atomic
10-
#' anyInfinite test_subset test_set_equal checkInt
10+
#' anyInfinite test_subset test_set_equal checkInt expect_class
1111
#' @importFrom cli cli_abort cli_warn
1212
#' @importFrom rlang %||%
1313
#' @name epiprocess

‎R/grouped_epi_archive.R‎

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -157,8 +157,7 @@ group_by.grouped_epi_archive <- function(
157157
#'
158158
#' @export
159159
group_by_drop_default.grouped_epi_archive <- function(.tbl) {
160-
x <- .tbl
161-
x$private$drop
160+
.tbl$private$drop
162161
}
163162

164163

‎R/methods-epi_archive.R‎

Lines changed: 0 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -797,9 +797,6 @@ epix_slide <- function(
797797
as_list_col = FALSE,
798798
names_sep = "_",
799799
all_versions = FALSE) {
800-
if (!is_epi_archive(x, grouped_okay = TRUE)) {
801-
cli_abort("`x` must be of class `epi_archive` or `grouped_epi_archive`.")
802-
}
803800
UseMethod("epix_slide")
804801
}
805802

‎man/epi_archive.Rd‎

Lines changed: 16 additions & 4 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎man/is_epi_archive.Rd‎

Lines changed: 0 additions & 35 deletions
This file was deleted.

‎tests/testthat/test-archive.R‎

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,3 @@
1-
library(dplyr)
2-
31
test_that("first input must be a data.frame", {
42
expect_error(as_epi_archive(c(1, 2, 3), compactify = FALSE),
53
regexp = "Must be of type 'data.frame'."

‎tests/testthat/test-autoplot.R‎

Lines changed: 0 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,4 @@
1-
library(dplyr)
2-
31
d <- as.Date("2020-01-01")
4-
52
raw_df_chr <- dplyr::bind_rows(
63
dplyr::tibble(geo_value = "ak", time_value = d + 1:5, value = "a"),
74
dplyr::tibble(geo_value = "al", time_value = d + 1:5, value = "d")

0 commit comments

Comments
 (0)