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

Author: dshemetov

DESCRIPTION

@@ -51,12 +51,10 @@ Suggests:
     covidcast,
     devtools,
     epidatr,
-    here,
     knitr,
     outbreaks,
     rmarkdown,
     testthat (>= 3.1.5),
-    waldo (>= 0.3.1),
     withr
 VignetteBuilder:
     knitr

NAMESPACE

@@ -64,7 +64,6 @@ export(filter)
 export(group_by)
 export(group_modify)
 export(growth_rate)
-export(is_epi_archive)
 export(is_epi_df)
 export(is_grouped_epi_archive)
 export(key_colnames)
@@ -78,6 +77,7 @@ export(rename)
 export(slice)
 export(ungroup)
 export(unnest)
+export(validate_epi_archive)
 importFrom(checkmate,anyInfinite)
 importFrom(checkmate,anyMissing)
 importFrom(checkmate,assert)
@@ -94,6 +94,7 @@ importFrom(checkmate,checkInt)
 importFrom(checkmate,check_atomic)
 importFrom(checkmate,check_data_frame)
 importFrom(checkmate,check_names)
+importFrom(checkmate,expect_class)
 importFrom(checkmate,test_set_equal)
 importFrom(checkmate,test_subset)
 importFrom(checkmate,vname)

R/archive.R

@@ -307,59 +307,8 @@ new_epi_archive <- function(
     other_keys = NULL,
     additional_metadata = NULL,
     compactify = NULL,
-    clobberable_versions_start = NA,
+    clobberable_versions_start = NULL,
     versions_end = NULL) {
-  assert_data_frame(x)
-  if (!test_subset(c("geo_value", "time_value", "version"), names(x))) {
-    cli_abort(
-      "Columns `geo_value`, `time_value`, and `version` must be present in `x`."
-    )
-  }
-  if (anyMissing(x$version)) {
-    cli_abort("Column `version` must not contain missing values.")
-  }
-
-  geo_type <- geo_type %||% guess_geo_type(x$geo_value)
-  time_type <- time_type %||% guess_time_type(x$time_value)
-  other_keys <- other_keys %||% character(0L)
-  additional_metadata <- additional_metadata %||% list()
-
-  # Finish off with small checks on keys variables and metadata
-  if (!test_subset(other_keys, names(x))) {
-    cli_abort("`other_keys` must be contained in the column names of `x`.")
-  }
-  if (any(c("geo_value", "time_value", "version") %in% other_keys)) {
-    cli_abort("`other_keys` cannot contain \"geo_value\", \"time_value\", or \"version\".")
-  }
-  if (any(names(additional_metadata) %in% c("geo_type", "time_type"))) {
-    cli_warn("`additional_metadata` names overlap with existing metadata fields \"geo_type\", \"time_type\".")
-  }
-
-  # Conduct checks and apply defaults for `compactify`
-  assert_logical(compactify, len = 1, any.missing = FALSE, null.ok = TRUE)
-
-  # Apply defaults and conduct checks for
-  # `clobberable_versions_start`, `versions_end`:
-  versions_end <- versions_end %||% max_version_with_row_in(x)
-  validate_version_bound(clobberable_versions_start, x, na_ok = TRUE)
-  validate_version_bound(versions_end, x, na_ok = FALSE)
-  if (nrow(x) > 0L && versions_end < max(x[["version"]])) {
-    cli_abort(
-      "`versions_end` was {versions_end}, but `x` contained
-        updates for a later version or versions, up through {max(x$version)}",
-      class = "epiprocess__versions_end_earlier_than_updates"
-    )
-  }
-  if (!is.na(clobberable_versions_start) && clobberable_versions_start > versions_end) {
-    cli_abort(
-      "`versions_end` was {versions_end}, but a `clobberable_versions_start`
-        of {clobberable_versions_start} indicated that there were later observed versions",
-      class = "epiprocess__versions_end_earlier_than_clobberable_versions_start"
-    )
-  }
-
-  # --- End of validation and replacing missing args with defaults ---
-
   # Create the data table; if x was an un-keyed data.table itself,
   # then the call to as.data.table() will fail to set keys, so we
   # need to check this, then do it manually if needed
@@ -441,18 +390,91 @@ new_epi_archive <- function(
   )
 }
 
+#' `validate_epi_archive` ensures correctness of arguments fed to `as_epi_archive`.
+#'
+#' @rdname epi_archive
+#'
+#' @export
+validate_epi_archive <- function(
+    x,
+    geo_type = NULL,
+    time_type = NULL,
+    other_keys = NULL,
+    additional_metadata = NULL,
+    compactify = NULL,
+    clobberable_versions_start = NULL,
+    versions_end = NULL) {
+  # Finish off with small checks on keys variables and metadata
+  if (!test_subset(other_keys, names(x))) {
+    cli_abort("`other_keys` must be contained in the column names of `x`.")
+  }
+  if (any(c("geo_value", "time_value", "version") %in% other_keys)) {
+    cli_abort("`other_keys` cannot contain \"geo_value\", \"time_value\", or \"version\".")
+  }
+  if (any(names(additional_metadata) %in% c("geo_type", "time_type"))) {
+    cli_warn("`additional_metadata` names overlap with existing metadata fields \"geo_type\", \"time_type\".")
+  }
+
+  # Conduct checks and apply defaults for `compactify`
+  assert_logical(compactify, len = 1, any.missing = FALSE, null.ok = TRUE)
+
+  # Apply defaults and conduct checks for
+  # `clobberable_versions_start`, `versions_end`:
+  validate_version_bound(clobberable_versions_start, x, na_ok = TRUE)
+  validate_version_bound(versions_end, x, na_ok = FALSE)
+  if (nrow(x) > 0L && versions_end < max(x[["version"]])) {
+    cli_abort(
+      "`versions_end` was {versions_end}, but `x` contained
+        updates for a later version or versions, up through {max(x$version)}",
+      class = "epiprocess__versions_end_earlier_than_updates"
+    )
+  }
+  if (!is.na(clobberable_versions_start) && clobberable_versions_start > versions_end) {
+    cli_abort(
+      "`versions_end` was {versions_end}, but a `clobberable_versions_start`
+        of {clobberable_versions_start} indicated that there were later observed versions",
+      class = "epiprocess__versions_end_earlier_than_clobberable_versions_start"
+    )
+  }
+}
+
 
 #' `as_epi_archive` converts a data frame, data table, or tibble into an
 #' `epi_archive` object.
 #'
 #' @rdname epi_archive
 #'
 #' @export
-as_epi_archive <- function(x, geo_type = NULL, time_type = NULL, other_keys = NULL,
-                           additional_metadata = list(),
-                           compactify = NULL,
-                           clobberable_versions_start = NA,
-                           versions_end = max_version_with_row_in(x)) {
+as_epi_archive <- function(
+    x,
+    geo_type = NULL,
+    time_type = NULL,
+    other_keys = NULL,
+    additional_metadata = NULL,
+    compactify = NULL,
+    clobberable_versions_start = NULL,
+    versions_end = NULL) {
+  assert_data_frame(x)
+  if (!test_subset(c("geo_value", "time_value", "version"), names(x))) {
+    cli_abort(
+      "Columns `geo_value`, `time_value`, and `version` must be present in `x`."
+    )
+  }
+  if (anyMissing(x$version)) {
+    cli_abort("Column `version` must not contain missing values.")
+  }
+
+  geo_type <- geo_type %||% guess_geo_type(x$geo_value)
+  time_type <- time_type %||% guess_time_type(x$time_value)
+  other_keys <- other_keys %||% character(0L)
+  additional_metadata <- additional_metadata %||% list()
+  clobberable_versions_start <- clobberable_versions_start %||% NA
+  versions_end <- versions_end %||% max_version_with_row_in(x)
+
+  validate_epi_archive(
+    x, geo_type, time_type, other_keys, additional_metadata,
+    compactify, clobberable_versions_start, versions_end
+  )
   new_epi_archive(
     x, geo_type, time_type, other_keys, additional_metadata,
     compactify, clobberable_versions_start, versions_end
@@ -652,31 +674,6 @@ group_by.epi_archive <- function(.data, ..., .add = FALSE, .drop = dplyr::group_
 }
 
 
-#' Test for `epi_archive` format
-#'
-#' @param x An object.
-#' @param grouped_okay Optional; Boolean; should a `grouped_epi_archive` also
-#'   count? Default is `FALSE`.
-#' @return `TRUE` if the object inherits from `epi_archive`.
-#'
-#' @export
-#' @examples
-#' is_epi_archive(jhu_csse_daily_subset) # FALSE (this is an epi_df, not epi_archive)
-#' is_epi_archive(archive_cases_dv_subset) # TRUE
-#'
-#' # By default, grouped_epi_archives don't count as epi_archives, as they may
-#' # support a different set of operations from regular `epi_archives`. This
-#' # behavior can be controlled by `grouped_okay`.
-#' grouped_archive <- archive_cases_dv_subset %>% group_by(geo_value)
-#' is_epi_archive(grouped_archive) # FALSE
-#' is_epi_archive(grouped_archive, grouped_okay = TRUE) # TRUE
-#'
-#' @seealso [`is_grouped_epi_archive`]
-is_epi_archive <- function(x, grouped_okay = FALSE) {
-  inherits(x, "epi_archive") || grouped_okay && inherits(x, "grouped_epi_archive")
-}
-
-
 #' Clone an `epi_archive` object.
 #'
 #' @param x An `epi_archive` object.

R/epiprocess.R

@@ -7,7 +7,7 @@
 #' @importFrom checkmate assert assert_scalar assert_data_frame anyMissing
 #'             assert_logical assert_list assert_character assert_class
 #'             assert_int assert_numeric check_data_frame vname check_atomic
-#'             anyInfinite test_subset test_set_equal checkInt
+#'             anyInfinite test_subset test_set_equal checkInt expect_class
 #' @importFrom cli cli_abort cli_warn
 #' @importFrom rlang %||%
 #' @name epiprocess

R/grouped_epi_archive.R

@@ -157,8 +157,7 @@ group_by.grouped_epi_archive <- function(
 #'
 #' @export
 group_by_drop_default.grouped_epi_archive <- function(.tbl) {
-  x <- .tbl
-  x$private$drop
+  .tbl$private$drop
 }
 
 

R/methods-epi_archive.R

@@ -797,9 +797,6 @@ epix_slide <- function(
     as_list_col = FALSE,
     names_sep = "_",
     all_versions = FALSE) {
-  if (!is_epi_archive(x, grouped_okay = TRUE)) {
-    cli_abort("`x` must be of class `epi_archive` or `grouped_epi_archive`.")
-  }
   UseMethod("epix_slide")
 }
 

man/epi_archive.Rd

@@ -1,5 +1,3 @@
-library(dplyr)
-
 test_that("first input must be a data.frame", {
   expect_error(as_epi_archive(c(1, 2, 3), compactify = FALSE),
     regexp = "Must be of type 'data.frame'."

man/is_epi_archive.Rd

@@ -1,7 +1,4 @@
-library(dplyr)
-
 d <- as.Date("2020-01-01")
-
 raw_df_chr <- dplyr::bind_rows(
   dplyr::tibble(geo_value = "ak", time_value = d + 1:5, value = "a"),
   dplyr::tibble(geo_value = "al", time_value = d + 1:5, value = "d")

tests/testthat/test-archive.R

@@ -1,7 +1,3 @@
-library(epiprocess)
-library(data.table)
-library(dplyr)
-
 dt <- archive_cases_dv_subset$DT
 dt <- filter(dt, geo_value == "ca") %>%
   filter(version <= "2020-06-15") %>%

tests/testthat/test-autoplot.R

@@ -1,5 +1,3 @@
-library(tibble)
-
 test_that("epi_cor throws an error for a non-epi_df for its first argument", {
   expect_error(epi_cor(1:10, 1, 1))
   expect_error(epi_cor(data.frame(x = 1:10), 1, 1))

tests/testthat/test-compactify.R

@@ -1,5 +1,5 @@
 test_that("`archive_cases_dv_subset` is formed successfully", {
-  expect_true(is_epi_archive(archive_cases_dv_subset))
+  expect_class(archive_cases_dv_subset, "epi_archive")
 })
 
 test_that("`delayed_assign_with_unregister_awareness` works as expected on good promises", {

tests/testthat/test-correlation.R

@@ -506,7 +506,7 @@ test_that("epix_as_of and epix_slide with long enough window are compatible", {
 
 test_that("epix_slide `f` is passed an ungrouped `epi_archive` when `all_versions=TRUE`", {
   slide_fn <- function(x, gk, rtv) {
-    expect_true(is_epi_archive(x))
+    expect_class(x, "epi_archive")
     return(NA)
   }
 

tests/testthat/test-data.R

@@ -1,7 +1,4 @@
-library(dplyr)
-
 ea <- archive_cases_dv_subset
-
 ea2_data <- tibble::tribble(
   ~geo_value, ~time_value, ~version, ~cases,
   "ca", "2020-06-01", "2020-06-01", 1,
@@ -104,7 +101,7 @@ test_that("epix_truncate_version_after returns the same grouping type as input e
 
   ea_as_of <- ea2 %>%
     epix_truncate_versions_after(max_version = as.Date("2020-06-04"))
-  expect_true(is_epi_archive(ea_as_of, grouped_okay = FALSE))
+  expect_class(ea_as_of, "epi_archive")
 
   ea2_grouped <- ea2 %>% group_by(geo_value)