Skip to content
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

[188605595]: Remove unused appliedFilter and deprecated variables ord… #651

Merged
merged 4 commits into from
Dec 19, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 0 additions & 5 deletions R/AllGenerics.R
Original file line number Diff line number Diff line change
Expand Up @@ -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<-",
Expand Down
13 changes: 0 additions & 13 deletions R/filters.R
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
83 changes: 8 additions & 75 deletions R/ordering.R
Original file line number Diff line number Diff line change
@@ -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
Expand Down Expand Up @@ -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)
})
Expand Down Expand Up @@ -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))
}
2 changes: 1 addition & 1 deletion R/weight.R
Original file line number Diff line number Diff line change
Expand Up @@ -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)) {
Expand Down
23 changes: 0 additions & 23 deletions man/copyOrderGroup.Rd

This file was deleted.

6 changes: 5 additions & 1 deletion man/ordering.Rd

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

14 changes: 0 additions & 14 deletions tests/testthat/test-filters.R
Original file line number Diff line number Diff line change
Expand Up @@ -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))
})
Expand Down
18 changes: 4 additions & 14 deletions tests/testthat/test-fork.R
Original file line number Diff line number Diff line change
Expand Up @@ -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", {
Expand Down Expand Up @@ -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)
Expand Down
Loading
Loading