diff --git a/R/AllGenerics.R b/R/AllGenerics.R index 45b355867..153e3ba31 100644 --- a/R/AllGenerics.R +++ b/R/AllGenerics.R @@ -246,11 +246,6 @@ setGeneric("multitables<-", function(x, value) standardGeneric("multitables<-")) setGeneric("filters", function(x) standardGeneric("filters")) #' @rdname filter-catalog setGeneric("filters<-", function(x, value) standardGeneric("filters<-")) -setGeneric("appliedFilters", function(x) standardGeneric("appliedFilters")) -setGeneric( - "appliedFilters<-", - function(x, value) standardGeneric("appliedFilters<-") -) setGeneric("activeFilter", function(x) standardGeneric("activeFilter")) setGeneric( "activeFilter<-", diff --git a/R/filters.R b/R/filters.R index 0ce902eb4..5e82cbb0c 100644 --- a/R/filters.R +++ b/R/filters.R @@ -141,19 +141,6 @@ setMethod( } ) -setMethod("appliedFilters", "CrunchDataset", function(x) { - out <- ShojiOrder(crGET(shojiURL(x, "views", "applied_filters"))) - return(out@graph) -}) - -setMethod( - "appliedFilters<-", c("CrunchDataset", "CrunchFilter"), - function(x, value) { - b <- list(graph = I(list(self(value)))) - crPUT(shojiURL(x, "views", "applied_filters"), body = toJSON(b)) - return(x) - } -) .getActiveFilter <- function(x) { f <- expr <- x@filter diff --git a/R/ordering.R b/R/ordering.R index 33c6650bd..1165359b8 100644 --- a/R/ordering.R +++ b/R/ordering.R @@ -1,8 +1,11 @@ #' Get and set VariableOrder #' -#' The `ordering` methods allow you to get and set a [`VariableOrder`] on a +#' The `ordering` methods allow you to get a [`VariableOrder`] on a #' [`CrunchDataset`] or on the [`VariableCatalog`] that the dataset contains. #' +#' Crunch datasets work with folders, and the ordering is deprecated. It is no +#' longer possible to set the ordering of a variable catalog from rcrunch. +#' #' @param x a VariableCatalog or CrunchDataset #' @param value a valid VariableOrder object #' @return `ordering` returns a VariableOrder object, while @@ -42,35 +45,13 @@ setMethod("ordering<-", "VariableCatalog", function(x, value) { stopifnot(inherits(value, "VariableOrder")) if (!identical(ordering(x)@graph, value@graph)) { - ## Give deprecation warning (the first time only per session) - warn_once( + halt( "Hey! There's a new way to organize variables within ", "datasets: the 'folder' methods. They're easier to use and ", "more reliable. See `?mv`, `?cd`, and others for details, and ", "`vignettes('variable-order', package='crunch')` for examples. ", - "You're seeing this message because you're still using the ", - "ordering<- method, which is fine today, but it will be going ", - "away in the future, so check out the new methods. ", - option = "crunch.already.shown.folders.msg" + "The old ordering<- method no longer works." ) - - ## Validate. - bad.entities <- setdiff(urls(value), urls(x)) - if (length(bad.entities)) { - halt( - pluralize("Variable URL", length(bad.entities)), - " referenced in Order not present in catalog: ", - serialPaste(bad.entities) - ) - } - - order_url <- shojiURL(x, "orders", "hier") - ## Update on server - crPUT(order_url, body = toJSON(value)) - ## Drop cache for dataset folders - dropCache(paste0(datasetReference(x), "folders/")) - ## Refresh - x@order <- VariableOrder(crGET(order_url)) } return(x) }) @@ -132,58 +113,10 @@ copyOrder <- function(source, target) { halt("Both source and target must be Crunch datasets.") } - warning( + halt( "There's a new way to copy ordering and folders: `copyFolders`!", "It uses Crunch's new folders system which is easier to use and more ", - "reliable. `copyOrder` will be removed shortly, so please change your ", + "reliable. `copyOrder` has been removed, so please change your ", "code to use `copyFolders`. See `?copyFolders` for more information." ) - - ord <- entities(ordering(source)) - - # make url and alias maps - url_to_alias_source <- as.list( - structure(aliases(allVariables(source)), .Names = urls(allVariables(source))) - ) - alias_to_url_target <- as.list( - structure(urls(allVariables(target)), .Names = aliases(allVariables(target))) - ) - - new_ord <- lapply(ord, copyOrderGroup, - source_map = url_to_alias_source, - target_map = alias_to_url_target - ) - - # drop any null entities, those that were not found in target but in source - new_ord <- removeMissingEntities(new_ord) - new_ord <- do.call(VariableOrder, new_ord) - - # set catalog URL so show methods work on the new ordering - new_ord@catalog_url <- variableCatalogURL(target) - - return(new_ord) -} - -#' Copy the order of a `VariableGroup` (or individual variable URL) from `VariableOrder` -#' -#' -#' @param group the group or variable URL to be copied -#' @param source_map url to alias map for source variables -#' @param target_map alias to url map for target variables -#' @return returns either a [`VariableGroup`] (if a group is supplied) or a URL -#' (if just a variable URL is supplied) -#' @keywords internal -copyOrderGroup <- function(group, source_map, target_map) { - # if there is a single element in group, and it is a character, - # just return the URL in the target. - if (length(group) == 1 & is.character(group)) { - return(target_map[[source_map[[group]]]] %||% NA_character_) - } - - # there are groups, so recurse - ents <- lapply(entities(group), copyOrderGroup, - source_map = source_map, target_map = target_map - ) - - return(VariableGroup(name(group), ents)) } diff --git a/R/weight.R b/R/weight.R index 38168d76c..6b0e224e6 100644 --- a/R/weight.R +++ b/R/weight.R @@ -157,7 +157,7 @@ modifyWeightVariables <- function(x, vars, type = "append") { ## variaous inputs into a list of variables. if (is.null(vars)) { # If NULL change type to replace to clear the weight variables - new$graph <- NULL + new$graph <- list() type <- "replace" } else { if (is.variable(vars) || (length(vars) == 1) & !is.character(vars)) { diff --git a/man/copyOrderGroup.Rd b/man/copyOrderGroup.Rd deleted file mode 100644 index ef6e81dec..000000000 --- a/man/copyOrderGroup.Rd +++ /dev/null @@ -1,23 +0,0 @@ -% Generated by roxygen2: do not edit by hand -% Please edit documentation in R/ordering.R -\name{copyOrderGroup} -\alias{copyOrderGroup} -\title{Copy the order of a \code{VariableGroup} (or individual variable URL) from \code{VariableOrder}} -\usage{ -copyOrderGroup(group, source_map, target_map) -} -\arguments{ -\item{group}{the group or variable URL to be copied} - -\item{source_map}{url to alias map for source variables} - -\item{target_map}{alias to url map for target variables} -} -\value{ -returns either a \code{\link{VariableGroup}} (if a group is supplied) or a URL -(if just a variable URL is supplied) -} -\description{ -Copy the order of a \code{VariableGroup} (or individual variable URL) from \code{VariableOrder} -} -\keyword{internal} diff --git a/man/ordering.Rd b/man/ordering.Rd index 02ad15867..938dd2984 100644 --- a/man/ordering.Rd +++ b/man/ordering.Rd @@ -43,7 +43,11 @@ ordering(x) <- value \verb{ordering<-} sets the VariableOrder } \description{ -The \code{ordering} methods allow you to get and set a \code{\link{VariableOrder}} on a +The \code{ordering} methods allow you to get a \code{\link{VariableOrder}} on a \code{\link{CrunchDataset}} or on the \code{\link{VariableCatalog}} that the dataset contains. } +\details{ +Crunch datasets work with folders, and the ordering is deprecated. It is no +longer possible to set the ordering of a variable catalog from rcrunch. +} \keyword{internal} diff --git a/tests/testthat/test-filters.R b/tests/testthat/test-filters.R index 688f4c300..926822117 100644 --- a/tests/testthat/test-filters.R +++ b/tests/testthat/test-filters.R @@ -258,20 +258,6 @@ with_test_authentication({ ) }) - test_that("We have an applied filters view", { - expect_length(appliedFilters(ds), 0) - }) - - test_that("We can 'apply' a filter", { - appliedFilters(ds) <- filters(ds)[["Test filter"]] - expect_length(appliedFilters(ds), 1) - }) - - test_that("'applied filters' for the UI don't affect R", { - expect_length(appliedFilters(ds), 1) - expect_valid_df_import(ds) - }) - test_that("We also have 'active filter' for the R object", { expect_null(activeFilter(ds)) }) diff --git a/tests/testthat/test-fork.R b/tests/testthat/test-fork.R index 5d80edf78..551dd491d 100644 --- a/tests/testthat/test-fork.R +++ b/tests/testthat/test-fork.R @@ -108,23 +108,17 @@ with_test_authentication({ # 2. Edit dataset metadata description(f1) <- "A dataset for testing" - # 3. Reorder variables - ordering(f1) <- VariableOrder( - VariableGroup("Even", f1[c(2, 4, 6)]), - VariableGroup("Odd", f1[c(1, 3, 5)]) - ) - - # 4. Add non-derived variable + # 3. Add non-derived variable f1$v8 <- rep(1:5, 4)[4:20] - # 5. Derive variable + # 4. Derive variable f1$v7 <- f1$v3 - 6 - # 6. Conditionally edit values of categorical variable + # 5. Conditionally edit values of categorical variable f1$v4[f1$v8 == 5] <- "F" f1$v4[f1$v8 == 4] <- "F" - # 7. Delete a variable and replace it with one of the same name + # 6. Delete a variable and replace it with one of the same name new_vect <- rev(as.vector(f1$v1)) v1copy <- VariableDefinition(new_vect, name = name(f1$v1), alias = alias(f1$v1)) test_that("Just asserting that the new var has the same name/alias as old", { @@ -154,10 +148,6 @@ with_test_authentication({ expect_identical(as.vector(dataset$v7), df$v3[4:20] - 6) expect_equivalent(as.vector(dataset$v8), rep(1:5, 4)[4:20]) expect_equivalent(as.vector(dataset$v1), rev(df$v1[4:20])) - expect_identical( - aliases(variables(dataset)), - paste0("v", c(2, 4, 6, 3, 5, 8, 7, 1)) - ) } test_that("The edits are made to the fork", { expect_fork_edits(f1) diff --git a/tests/testthat/test-variable-order.R b/tests/testthat/test-variable-order.R index a347f7f2e..b538acd9f 100644 --- a/tests/testthat/test-variable-order.R +++ b/tests/testthat/test-variable-order.R @@ -55,16 +55,10 @@ with_mock_crunch({ }) test_that("Warning that you should be using folders instead", { - set_crunch_opts(crunch.already.shown.folders.msg = NULL) - expect_warning( - expect_PUT(ordering(ds) <- nested.ord[2:1]), + expect_error( + ordering(ds) <- nested.ord[2:1], "Hey!" ) - ## Second time it doesn't warn. One nag per session - expect_warning( - expect_PUT(ordering(ds) <- nested.ord[2:1]), - NA - ) }) test_that("length methods", { @@ -621,15 +615,12 @@ with_mock_crunch({ ) }) - test_that("copyOrder returns the order of target as a VariableOrder", { + test_that("copyOrder has been removed", { ds_again <- cachedLoadDataset("test ds") - # because copyOrder is deprecated, there will be a warning. - expect_warning( + expect_error( new_order <- copyOrder(ds, ds_again), "There's a new way to copy ordering and folders: `copyFolders`!" ) - expect_is(new_order, "VariableOrder") - expect_identical(entities(ordering(ds)), entities(new_order)) }) test_that("copyOrder input validation", { @@ -649,323 +640,4 @@ with_test_authentication({ urls(allVariables(ds)) )) }) - test_that("Can construct VariableOrder from variables", { - # TODO: probably covered by unit tests - vg <- VariableOrder( - VariableGroup( - name = "Group 1", - variables = ds[c("v1", "v3", "v5")] - ), - VariableGroup(name = "Group 2.5", entities = ds["v4"]), - VariableGroup( - name = "Group 2", - entities = ds[c("v6", "v2")] - ) - ) - vglist <- cereal(vg) - expect_identical(vglist, list(graph = list( - list(`Group 1` = list(self(ds$v1), self(ds$v3), self(ds$v5))), - list(`Group 2.5` = list(self(ds$v4))), - list(`Group 2` = list(self(ds$v6), self(ds$v2))) - ))) - }) - starting.vg <- vg <- VariableOrder( - VariableGroup( - name = "Group 1", - entities = ds[c("v1", "v3", "v5")] - ), - VariableGroup(name = "Group 2.5", variables = ds["v4"]), - VariableGroup( - name = "Group 2", - entities = ds[c("v6", "v2")] - ) - ) - - try(entities(vg[[2]]) <- self(ds$v2)) - test_that("Set URLs -> entities on VariableGroup", { - # TODO: move to unit test - expect_identical(urls(vg[[2]]), self(ds$v2)) - expect_identical( - urls(vg), - c( - self(ds$v1), self(ds$v3), self(ds$v5), self(ds$v2), - self(ds$v6) - ) - ) - }) - try(entities(vg[[2]]) <- list(ds$v3)) - test_that("Set variables -> entities on VariableGroup", { - # TODO: move to unit test - expect_identical(urls(vg[[2]]), self(ds$v3)) - }) - - try(name(vg[[2]]) <- "Group 3") - test_that("Set name on VariableGroup", { - # TODO: move to unit test - expect_identical(names(vg), c("Group 1", "Group 3", "Group 2")) - }) - try(names(vg) <- c("G3", "G1", "G2")) - test_that("Set names on VariableOrder", { - # TODO: move to unit test - expect_identical(names(vg), c("G3", "G1", "G2")) - }) - - original.order <- ordering(ds) - test_that("Can set VariableOrder on dataset", { - expect_false(identical(starting.vg, original.order)) - ordering(ds) <- starting.vg - expect_identical( - entities(grouped(ordering(ds))), - entities(starting.vg) - ) - expect_identical( - entities(grouped(ordering(refresh(ds)))), - entities(starting.vg) - ) - expect_is(ungrouped(ordering(ds)), "VariableGroup") - expect_is(ungrouped(ordering(refresh(ds))), "VariableGroup") - expect_identical( - names(ordering(ds)), - c("Group 1", "Group 2.5", "Group 2") - ) - - ## Test that can reorder groups - ordering(ds) <- starting.vg[c(2, 1, 3)] - expect_identical( - entities(grouped(ordering(ds))), - entities(starting.vg[c(2, 1, 3)]) - ) - expect_identical( - names(ordering(ds)), - c("Group 2.5", "Group 1", "Group 2") - ) - expect_identical( - names(ordering(refresh(ds))), - c("Group 2.5", "Group 1", "Group 2") - ) - - ds <- refresh(ds) - expect_false(identical( - entities(ordering(variables(ds))), - entities(original.order) - )) - ordering(variables(ds)) <- original.order - expect_identical( - entities(ordering(variables(ds))), - entities(original.order) - ) - expect_identical( - entities(ordering(variables(refresh(ds)))), - entities(original.order) - ) - }) - - test_that("A partial order results in 'ungrouped' variables", { - ordering(ds) <- starting.vg[1:2] - expect_is(grouped(ordering(ds)), "VariableOrder") - expect_identical( - entities(grouped(ordering(ds))), - entities(starting.vg[1:2]) - ) - expect_is(ungrouped(ordering(ds)), "VariableGroup") - expect_true(setequal( - unlist(entities(ungrouped(ordering(ds)))), - c(self(ds$v6), self(ds$v2)) - )) - }) - - test_that("grouped and ungrouped within a group", { - nesting <- VariableGroup("Nest", self(ds$v3)) - ordering(ds) <- starting.vg - ordering(ds)[["Group 1"]][[2]] <- nesting - ## Update fixture with duplicates=TRUE, as it should be found - ## after setting on a duplicates=TRUE order - expect_identical( - grouped(ordering(ds)[["Group 1"]]), - VariableGroup("Group 1", list(nesting)) - ) - expect_identical( - ungrouped(ordering(ds)[["Group 1"]]), - VariableGroup("ungrouped", list(self(ds$v1), self(ds$v5))) - ) - }) - - test_that("Can manipulate VariableOrder that's part of a dataset", { - ordering(ds) <- starting.vg - expect_identical( - names(ordering(ds)), - c("Group 1", "Group 2.5", "Group 2") - ) - names(ordering(ds))[3] <- "Three" - expect_identical( - names(ordering(ds)), - c("Group 1", "Group 2.5", "Three") - ) - expect_identical( - names(grouped(ordering(ds))), - c("Group 1", "Group 2.5", "Three") - ) - }) - - test_that("ordering<- validation", { - # TODO: move to unit test - bad.vg <- starting.vg - entities(bad.vg[[1]]) <- c( - entities(bad.vg[[1]])[-2], - "/not/a/variable" # nolint - ) - expect_error( - ordering(ds) <- bad.vg, - "Variable URL referenced in Order not present in catalog: /not/a/variable" - ) - }) - - test_that("Creating VariableOrder with named list doesn't break", { - bad.vg <- do.call(VariableOrder, c(sapply(names(starting.vg), - function(i) starting.vg[[i]], - simplify = FALSE - ))) - ## The list of entities is named because sapply default is - ## USE.NAMES=TRUE, but the VariableOrder constructor should - ## handle this - ordering(ds) <- bad.vg - expect_identical(ordering(ds)@graph, starting.vg@graph) - }) - - test_that("copyOrder copies across datasets with simple order", { - ds_fork <- forkDataset(ds) - old_order <- ordering(ds_fork) - new_order <- VariableOrder( - self(ds$v1), self(ds$v2), self(ds$v5), - self(ds$v6), self(ds$v3), self(ds$v4) - ) - new_order_fork <- VariableOrder( - self(ds_fork$v1), self(ds_fork$v2), - self(ds_fork$v5), self(ds_fork$v6), - self(ds_fork$v3), self(ds_fork$v4) - ) - ordering(ds) <- new_order - - # test that ds has the new order - expect_identical(entities(ordering(ds)), entities(new_order)) - # test that ds_fork has the old order still - expect_identical(entities(ordering(ds_fork)), entities(old_order)) - expect_false(identical(entities(ordering(ds_fork)), entities(new_order_fork))) - - # copy order, and check that ds_fork has the new order. - expect_warning(copied_order <- copyOrder(ds, ds_fork)) - ordering(ds_fork) <- copied_order - expect_identical(entities(ordering(ds_fork)), entities(new_order_fork)) - }) - - test_that("copyOrder copies across datasets with simple(-ish) order (and one nesting)", { - ds_fork <- forkDataset(ds) - old_order <- ordering(ds_fork) - new_order <- VariableOrder( - self(ds$v1), self(ds$v2), self(ds$v5), - self(ds$v6), VariableGroup( - "Group A", - list(self(ds$v4), self(ds$v3)) - ) - ) - new_order_fork <- VariableOrder( - self(ds_fork$v1), self(ds_fork$v2), - self(ds_fork$v5), self(ds_fork$v6), - VariableGroup( - "Group A", - list(self(ds_fork$v4), self(ds_fork$v3)) - ) - ) - ordering(ds) <- new_order - - # test that ds has the new order - expect_identical(entities(ordering(ds)), entities(new_order)) - # test that ds_fork has the old order still - expect_identical(entities(ordering(ds_fork)), entities(old_order)) - expect_false(identical(entities(ordering(ds_fork)), entities(new_order_fork))) - - # copy order, and check that ds_fork has the new order. - expect_warning(copied_order <- copyOrder(ds, ds_fork)) - ordering(ds_fork) <- copied_order - expect_identical(entities(ordering(ds_fork)), entities(new_order_fork)) - }) - - - test_that("copyOrder copies across datasets with nested hierarchical order", { - ds_fork <- forkDataset(ds) - old_order <- ordering(ds_fork) - new_order <- VariableOrder( - VariableGroup("Group 1", list( - self(ds$v1), self(ds$v2), - VariableGroup("Group 1.5", list(self(ds$v5), self(ds$v6))) - )), - VariableGroup("Group 2", list(self(ds$v4), self(ds$v3))) - ) - new_order_fork <- VariableOrder( - VariableGroup("Group 1", list( - self(ds_fork$v1), self(ds_fork$v2), - VariableGroup("Group 1.5", list(self(ds_fork$v5), self(ds_fork$v6))) - )), - VariableGroup("Group 2", list(self(ds_fork$v4), self(ds_fork$v3))) - ) - ordering(ds) <- new_order - - # test that ds has the new order - expect_identical(entities(ordering(ds)), entities(new_order)) - # test that ds_fork has the old order still - expect_identical(entities(ordering(ds_fork)), entities(old_order)) - expect_false(identical(entities(ordering(ds_fork)), entities(new_order_fork))) - - # copy order, and check that ds_fork has the new order. - expect_warning(copied_order <- copyOrder(ds, ds_fork)) - ordering(ds_fork) <- copied_order - expect_identical(entities(ordering(ds_fork)), entities(new_order_fork)) - }) - - test_that("copyOrder copies across disparate datasets", { - # setup an alternative dataset that has some overlap with ds - df_alt <- df - df_alt$v12 <- df_alt$v1 - df_alt$v1 <- NULL - df_alt$v2 <- NULL - df_alt$new_var <- 1 - df_alt$new_var2 <- letters[20:1] - ds_alt <- newDataset(df_alt) - - old_order <- ordering(ds_alt) - new_order <- VariableOrder( - self(ds$v1), self(ds$v2), self(ds$v5), - self(ds$v6), VariableGroup( - "Group A", - list(self(ds$v4), self(ds$v3)) - ) - ) - new_order_alt <- VariableOrder( - self(ds_alt$v5), self(ds_alt$v6), - VariableGroup( - "Group A", - list(self(ds_alt$v4), self(ds_alt$v3)) - ), - # the following variables do not overlap with ds, - # and therefor will be appended to the end, - # but their order will not be garuanteed - self(ds_alt$v12), self(ds_alt$new_var), self(ds_alt$new_var2) - ) - ordering(ds) <- new_order - - # test that ds has the new order - expect_identical(entities(ordering(ds)), entities(new_order)) - # test that ds_alt has the old order still - expect_identical(entities(ordering(ds_alt)), entities(old_order)) - expect_false(identical(entities(ordering(ds_alt)), entities(new_order_alt))) - - # copy order, and check that ds_alt has the new order. - expect_warning(copied_order <- copyOrder(ds, ds_alt)) - ordering(ds_alt) <- copied_order - # ignore the last three variables because their order was not specified - expect_identical( - entities(ordering(ds_alt))[-c(4, 5, 6)], - entities(new_order_alt)[-c(4, 5, 6)] - ) - }) }) diff --git a/tests/testthat/test-versioning.R b/tests/testthat/test-versioning.R index 806bc0986..c83142b5f 100644 --- a/tests/testthat/test-versioning.R +++ b/tests/testthat/test-versioning.R @@ -106,10 +106,7 @@ with_test_authentication({ # No longer supported # 3. Reorder variables - ordering(ds) <- VariableOrder( - VariableGroup("Even", ds[c(2, 4, 6)]), - VariableGroup("Odd", ds[c(1, 3, 5)]) - ) + # No longer supported # 4. Derive variable ds$v7 <- ds$v3 - 6 @@ -127,10 +124,6 @@ with_test_authentication({ ) expect_identical(as.vector(ds$v7), df$v3 - 6) expect_equivalent(as.vector(ds$v8), rep(1:5, 4)) - expect_identical( - aliases(variables(ds)), - paste0("v", c(2, 4, 6, 1, 3, 5, 7, 8)) - ) }) ## Assert those things again @@ -143,10 +136,7 @@ with_test_authentication({ ) expect_identical(as.vector(ds$v7), df$v3 - 6) expect_equivalent(as.vector(ds$v8), rep(1:5, 4)) - expect_identical( - aliases(variables(ds)), - paste0("v", c(2, 4, 6, 1, 3, 5, 7, 8)) - ) + }) ## Save a version @@ -170,13 +160,6 @@ with_test_authentication({ expect_valid_df_revert(ds) }) - test_that("Added variables are really removed by rolling back", { - ## This was user-reported: Order was reverted but derived - ## variables persisted, and by assigning an empty order, you can - ## recover them. - ordering(ds) <- VariableOrder() - expect_true(setequal(names(ds), names(df))) - }) test_that("And now we can add variables again that we added and reverted", { expect_null(ds$v7)